Closed
Bug 957201
Opened 11 years ago
Closed 2 years ago
Define and use NS_WARN_IF_FAILED
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: mccr8, Unassigned)
Details
Attachments
(1 file)
(deleted),
patch
|
benjamin
:
review-
|
Details | Diff | Splinter Review |
Per the dev.platform discussion "Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS".
Reporter | ||
Comment 1•11 years ago
|
||
try run just in case: https://tbpl.mozilla.org/?tree=Try&rev=5336ef8ef3a2
Attachment #8356675 -
Flags: review?(benjamin)
Reporter | ||
Comment 2•11 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•11 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•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Comment 4•10 years ago
|
||
So, is there any way to log the error value to the console? Because otherwise I still prefer NS_ENSURE_SUCCESS().
Reporter | ||
Comment 5•10 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.
Comment 6•10 years ago
|
||
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.
Comment 7•10 years ago
|
||
(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.
Comment 10•10 years ago
|
||
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•10 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•10 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•10 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•10 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.
Comment 15•10 years ago
|
||
(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?
Updated•2 years ago
|
Severity: normal → S3
Reporter | ||
Updated•2 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 2 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•