Define and use NS_WARN_IF_FAILED

REOPENED
Unassigned

Status

()

Core
XPCOM
REOPENED
5 years ago
11 months ago

People

(Reporter: mccr8, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Per the dev.platform discussion "Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS".
(Reporter)

Comment 1

5 years ago
Created attachment 8356675 [details] [diff] [review]
Define and use NS_WARN_IF_FAILED.

try run just in case: https://tbpl.mozilla.org/?tree=Try&rev=5336ef8ef3a2
Attachment #8356675 - Flags: review?(benjamin)
(Reporter)

Comment 2

5 years ago
This patch should use NS_WARN_IF_FAILED everywhere it can be used, at least as of the last time MXR  updated.

Comment 3

5 years ago
Comment on attachment 8356675 [details] [diff] [review]
Define and use NS_WARN_IF_FAILED.

Per dev.platform discussion, I think it's confusing to have a macro with a similar name but not be a straight passthrough. I'd prefer the more verbose version.
Attachment #8356675 - Flags: review?(benjamin) → review-

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
So, is there any way to log the error value to the console?  Because otherwise I still prefer NS_ENSURE_SUCCESS().
(Reporter)

Comment 5

4 years ago
I think

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

will do the same thing as NS_ENSURE_SUCCESS(), as the NS_WARN_IF() does the same warning thing.  It is just more clear that it is going to return.
With if (NS_WARN_IF(NS_FAILED(rv))) I don't see the |rv| _value_ in the console.  That is what I'm asking for.
(In reply to Honza Bambas (:mayhemer) from comment #6)
> With if (NS_WARN_IF(NS_FAILED(rv))) I don't see the |rv| _value_ in the
> console.  That is what I'm asking for.

That's actually a pretty compelling point here.
(In reply to Honza Bambas (:mayhemer) from comment #6)
> With if (NS_WARN_IF(NS_FAILED(rv))) I don't see the |rv| _value_ in the
> console.  That is what I'm asking for.

TBH I've only found that useful maybe once or twice. The rv is almost always something that could have been thrown from multiple places and then I have to get out the debugger anyway...
I know bz and I have found that useful before.
More generally - Over the last few months, I've determined that the ergonomics of if (NS_WARN_IF(NS_FAILED(rv)) stink, and generally result in less of this instrumentation in our codebase, even when I'm the one writing the code. :-(

I confess to having just used NS_ENSURE_SUCCESS anyway on a number of occasions.

Comment 11

4 years ago
ok, I agree that printing the rv is occasionally useful. I'd take a patch for:

if (NS_FAILED_WARNING(rv))
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 12

4 years ago
I'm probably not going to be able to get to this soon, so somebody else should feel free to take it.
Assignee: continuation → nobody

Comment 13

4 years ago
(In reply to comment #11)
> ok, I agree that printing the rv is occasionally useful. I'd take a patch for:
> 
> if (NS_FAILED_WARNING(rv))

Note that won't address comment 10.  I still think that NS_ENSURE_SUCCESS is better than all of the alternatives we have here.

Comment 14

4 years ago
We're not going to reach consensus, and I don't intend to reopen that discussion here. The style decision that Brendan oked was to avoid hiding returns.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #14)
> We're not going to reach consensus, and I don't intend to reopen that
> discussion here. The style decision that Brendan oked was to avoid hiding
> returns.

I get all that. but TBH, this is thus far the one stylistic decree that I (and from what I can gather, many of my peers in deep Gecko) haven't been able to stomach in a lot of places, and thus have been kinda-sorta ignoring. I believe in stylistic unification, and am making a real effort here. But it doesn't feel like it's sticking.

I wondering if just renaming NS_ENSURE_SUCCESS/NS_ENSURE_TRUE to WARN_AND_RETURN_IF_FAILED/WARN_AND_RETURN_IF_FALSE would handle 80% of the concerns leveled against this paradigm?
You need to log in before you can comment on or make changes to this bug.