Closed Bug 957201 Opened 10 years ago Closed 1 year ago

Define and use NS_WARN_IF_FAILED

Categories

(Core :: XPCOM, defect)

defect

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: mccr8, Unassigned)

Details

Attachments

(1 file)

Per the dev.platform discussion "Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS".
This patch should use NS_WARN_IF_FAILED everywhere it can be used, at least as of the last time MXR  updated.
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-
Status: NEW → RESOLVED
Closed: 10 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().
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.
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 → ---
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
(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.
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?
Severity: normal → S3
Status: REOPENED → RESOLVED
Closed: 10 years ago1 year ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: