See bug 1120059 comment 2.

Comment on attachment 8549031 [details] [diff] [review] Part 1: Prepare code for explicit bool operators Review of attachment 8549031 [details] [diff] [review]: ----------------------------------------------------------------- Frankly I much prefer != nullptr for null-checking things, but meh. ::: js/src/jsapi-tests/testGCCellPtr.cpp @@ +51,5 @@ > CHECK(GivesAndTakesCells(objcell)); > CHECK(GivesAndTakesCells(scriptcell)); > > JS::GCCellPtr copy = objcell; > + CHECK(copy.unsafeAsUIntPtr() == objcell.unsafeAsUIntPtr()); Rather than this, I think we want added inline bool operator==(const GCCellPtr &ptr1, const GCCellPtr &ptr2) { return ptr1.unsafeAsUIntPtr() == ptr2.unsafeAsUIntPtr(); } inline bool operator!=(const GCCellPtr &ptr1, const GCCellPtr &ptr2) { return !(ptr1 == ptr2); } to js/public/HeapAPI.h just beneath the GCCellPtr definition.

Comment on attachment 8549032 [details] [diff] [review] Part 2: Replace ConvertibleToBool hackarounds with explicit bool operators Review of attachment 8549032 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/imgFrame.h @@ +399,5 @@ > mRef = Move(aOther.mRef); > return *this; > } > > + explicit operator bool() const { return !!mFrame; } This code clearly preferred bool(mFrame) for this, so use that. @@ +479,5 @@ > > return *this; > } > > + explicit operator bool() const { return !!mFrame; } Same for this. ::: js/public/Debug.h @@ +172,2 @@ > // If we ever instantiate BuiltThink<Value>, this might not suffice. > + return !!value; Why can't we just do |return value;| here? (And fix the BuiltThin*g* typo while you're in the area.) ::: js/public/HashTable.h @@ +209,5 @@ > > /************************************************** Shorthand operations */ > > bool has(const Lookup &l) const { > + return !!impl.lookup(l); Use .found() for this. @@ +437,5 @@ > > /************************************************** Shorthand operations */ > > bool has(const Lookup &l) const { > + return !!impl.lookup(l); Ditto. ::: js/public/HeapAPI.h @@ +180,2 @@ > MOZ_ASSERT(bool(asCell()) == (kind() != JSTRACE_NULL)); > + return !!asCell(); Just asCell(). Or != nullptr if you must, but there's no need for it at all as it's just a Cell* ::: js/public/UbiNode.h @@ +301,5 @@ > bool operator==(const Node &rhs) const { return *base() == *rhs.base(); } > bool operator!=(const Node &rhs) const { return *base() != *rhs.base(); } > > + explicit operator bool() const { > + return !!base()->ptr; != nullptr Although, why does the return bit here even need to change? ptr is a void*, making the operator explicit should only affect *users* of it. ::: js/src/gc/GCInternals.h @@ +74,5 @@ > static IncrementalSafety Safe() { return IncrementalSafety(nullptr); } > static IncrementalSafety Unsafe(const char *reason) { return IncrementalSafety(reason); } > > + explicit operator bool() const { > + return !reason_; Leave this condition as it was before, reason_ == nullptr. ::: js/src/vm/ArrayBufferObject.h @@ +190,5 @@ > > uint8_t *data() const { return data_; } > BufferKind kind() const { return kind_; } > > + explicit operator bool() const { return !!data_; } data_ != nullptr ::: js/src/vm/Debugger.h @@ +128,5 @@ > > bool hasKeyInZone(JS::Zone *zone) { > CountMap::Ptr p = zoneCounts.lookup(zone); > MOZ_ASSERT_IF(p, p->value() > 0); > + return !!p; p.found() Also substitute p.found() into the assertion just above -- much more readable that way, versus "implicitly" checking the same condition. ::: js/src/vm/StructuredClone.cpp @@ +904,5 @@ > JSStructuredCloneWriter::startObject(HandleObject obj, bool *backref) > { > /* Handle cycles in the object graph. */ > CloneMemory::AddPtr p = memory.lookupForAdd(obj); > + if ((*backref = !!p)) Use the found() method instead. |if (p)| is barely readable, only as an idiom. Once you start assigning it into things, it's time to give up and use the named method. CloneMemory::AddPtr p = memory.lookupForAdd(obj); ; if ((*backref = p.found())) ... ::: mfbt/Range.h @@ +32,5 @@ > size_t length() const { return mEnd - mStart; } > > T& operator[](size_t aOffset) const { return mStart[aOffset]; } > > + explicit operator bool() const { return !!mStart; } mStart != nullptr ::: mfbt/RangedPtr.h @@ +114,5 @@ > } > > T* get() const { return mPtr; } > > + explicit operator bool() const { return !!mPtr; } mPtr != nullptr ::: mfbt/UniquePtr.h @@ +288,5 @@ > MOZ_ASSERT(get(), "dereferencing a UniquePtr containing nullptr"); > return get(); > } > > + explicit operator bool() const { return !!get(); } get() != nullptr. This matters: C++11 defines the result of this as != nullptr, which when we get around to supporting bug 1035966 will be an observable difference. @@ +428,5 @@ > reset(); > return *this; > } > > + explicit operator bool() const { return !!get(); } Again get() != nullptr, same reason.

Comments addressed. Sorry about the !! abuse. My past life with VC++ and its "forcing value to bool 'true' or 'false' (performance warning)" warning has taught me bad habits.

Note that the style guide explicitly disavows ==/!= nullptr.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5adb9f9867a https://hg.mozilla.org/integration/mozilla-inbound/rev/de42116d5ef3

https://hg.mozilla.org/mozilla-central/rev/b5adb9f9867a https://hg.mozilla.org/mozilla-central/rev/de42116d5ef3