Replace ConvertibleToBool hackarounds with explicit bool operators

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: poiru, Assigned: poiru)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Attachment #8549031 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8549032 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #8549032 - Attachment is obsolete: true
Attachment #8550348 - Flags: review?(jwalden+bmo)
Attachment #8550348 - Flags: review?(jwalden+bmo) → review+
Note that the style guide explicitly disavows ==/!= nullptr.
https://hg.mozilla.org/mozilla-central/rev/b5adb9f9867a
https://hg.mozilla.org/mozilla-central/rev/de42116d5ef3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.