Closed Bug 1153348 Opened 5 years ago Closed 5 years ago

Detect |operator bool| methods not marked as explicit

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jandem, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

We require one-argument constructors to be marked as either explicit or MOZ_IMPLICIT (bug 1009631). We should probably do the same for |operator bool|.

The "motivation" section here explains why explicit operator bool is less error-prone:

http://en.wikibooks.org/wiki/More_C++_Idioms/Safe_bool
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)
Comment on attachment 8591257 [details] [diff] [review]
Add an analysis to prohibit operator bools which aren't marked as either explicit or MOZ_IMPLICIT

Review of attachment 8591257 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/include/pkix/ScopedPtr.h
@@ +24,5 @@
>  
>  #ifndef mozilla_pkix_ScopedPtr_h
>  #define mozilla_pkix_ScopedPtr_h
>  
> +#include "mozilla/Attributes.h"

It's important that security/pkix has be able to build without mozilla/Atrributes. I'd rather just remove the operator bool() and change the checks to "!= nullptr" and "== nullptr" respectively.
Attachment #8591257 - Flags: review-
I made mozilla::pkix's ScopedPtr's operator bool explicit in bug 1153738. Please rebase this on top of that change. Thanks!
Depends on: 1153738
This is the counterpart to the existing analysis to catch
constructors which aren't marked as either explicit or
MOZ_IMPLICIT.
Attachment #8591257 - Attachment is obsolete: true
Attachment #8591257 - Flags: review?(jmuizelaar)
Attachment #8592280 - Flags: review?(jmuizelaar)
Comment on attachment 8592280 [details] [diff] [review]
Add an analysis to prohibit operator bools which aren't marked as either explicit or MOZ_IMPLICIT

Review of attachment 8592280 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +1675,5 @@
>    } else if (mDecoder) {
>      mDecoder->Pause();
>    }
>  
> +  bool oldPaused = static_cast<bool>(mPaused);

This goes away with the MOZ_IMPLICIT

::: dom/html/HTMLMediaElement.h
@@ +427,5 @@
>    }
>  
>    bool Paused() const
>    {
> +    return static_cast<bool>(mPaused);

This goes away

@@ +659,5 @@
>  
>      void SetOuter(HTMLMediaElement* outer) { mOuter = outer; }
>      void SetCanPlay(bool aCanPlay);
>  
> +    explicit operator bool() const { return mValue; }

MOZ_IMPLICIT is better

::: dom/media/GraphDriver.h
@@ +393,5 @@
>    }
>  
>    bool IsSwitchingDevice() {
>  #ifdef XP_MACOSX
> +    return static_cast<bool>(mSelfReference);

This goes away with the MOZ_IMPLICIT

::: dom/media/SelfRef.h
@@ +34,5 @@
>        t->Release();
>      }
>    }
>  
> +  explicit operator bool() const { return mHeld; }

we have MOZ_IMPLICIT on nsRefPtr. It makes sense to have it here too.
Attachment #8592280 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/192e5d94b106
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.