Closed Bug 1299727 Opened 4 years ago Closed 4 years ago

Rename NS_WARN_IF_FALSE as NS_WARNING_ASSERTION

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(1 file)

The new name makes the sense of the condition much clearer. E.g. compare:

  NS_WARN_IF_FALSE(!rv.Failed());

with:

  NS_WARNING_ASSERTION(!rv.Failed());

The new name also makes it clearer that it only has effect in debug builds,
because that's standard for assertions.
I'd love to use NS_WARN_ASSERTION or NS_WARNING_ASSERT or even NS_WARN_ASSERT
to reduce the number of line breaks required... but NS_WARNING_ASSERTION really
feels like the best name for this.
Attachment #8787054 - Flags: review?(erahm)
Comment on attachment 8787054 [details] [diff] [review]
Rename NS_WARN_IF_FALSE as NS_WARNING_ASSERTION

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

r=me

We're definitely going to need to send out a message to dev-platform for this as well as update any MDN docs.

::: dom/ipc/Blob.cpp
@@ +1604,5 @@
>      nsCOMPtr<nsIThread> ioTarget;
>      mIOTarget.swap(ioTarget);
>  
> +    NS_WARNING_ASSERTION(NS_SUCCEEDED(stream->Close()),
> +                         "Failed to close stream!");

Follow up: stream->Close() in debug only assert.
Attachment #8787054 - Flags: review?(erahm) → review+
> > +    NS_WARNING_ASSERTION(NS_SUCCEEDED(stream->Close()),
> > +                         "Failed to close stream!");
> 
> Follow up: stream->Close() in debug only assert.

Oh snap! Nice catch. That adds weight to the "clearer that it only has effect in debug builds" argument from comment 0. In fact, bug 994190 did exactly this:

> -    nsresult rv = stream->Close();
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    NS_WARN_IF_FALSE(NS_SUCCEEDED(stream->Close()), "Failed to close stream!");

<sadface>

I filed bug 1300007 to fix it.
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93657f8dedaa
Rename NS_WARN_IF_FALSE as NS_WARNING_ASSERTION. r=erahm.
https://hg.mozilla.org/mozilla-central/rev/93657f8dedaa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1272010
You need to log in before you can comment on or make changes to this bug.