Gecko assertions, errors, warnings, et al
Nicholas Nethercote
I blogged about the non-fatalness of Gecko assertions here:
http://blog.mozilla.com/nnethercote/2011/06/07/what-is-the-point-of-non-fatal-assertions/
I got some things wrong in the post, but the discussion was very
interesting. Robert O'Callahan said the discussion should have taken
place in dev-platform, so here's a follow-up where I will clarify my
thinking and make some suggestions.
Note that I'm a Gecko newbie. This means that I am unaware of a lot
of the history and subtleties in this issue, but I also am not yet
desensitized to weird and sub-optimal things.
THE CURRENT SITUATION
There are four "exceptional" events that can happen. These events and
their effects are:
- WARNING: prints a message to stderr.
- ASSERTION: prints a message to stderr, is detected by fuzzers,
causes test failures for some test suites (<a
href="https://bugzilla.mozilla.org/show_bug.cgi?id=404077">but not
Mochitests</a>). Pops up a dialog on Windows?
- BREAK: causes a debugger break?
- ABORT: aborts the process.
There are various macros that trigger NS_DebugBreak calls. The
numbers in parens show how many times each one appears in the code
(according to a grep search I did that may not be right, but gives you
a rough idea):
Macros that do stuff in DEBUG builds only:
- NS_ABORT_IF_FALSE (2049): triggers an ABORT with the failing
condition as the message.
- NS_WARN_IF_FALSE (152): triggers a WARNING with the failing
condition as the message.
- NS_ASSERTION (9928), NS_PRECONDITION (1966), NS_POSTCONDITION (47):
trigger an ERROR with the failing condition as the message.
- NS_NOTYETIMPLEMENTED (56), NS_NOTREACHED (762), NS_ERROR (1893):
trigger an ERROR with "NotYetImplemented", "Not Reached" and "Error"
as the message, respectively.
- NS_WARNING (3358): trigger a WARNING with the given argument as the message.
- NS_ABORT (7): trigger an ABORT with no specific message.
- NS_BREAK (8): trigger BREAK with no specific message.
Macros that do stuff in all builds:
- NS_RUNTIMEABORT (2930): triggers an ABORT with no specific message
- NS_ENSURE_TRUE (5048), NS_ENSURE_FALSE (59), NS_ENSURE_SUCCESS
(9637), and a bunch of other more specialized ones: if the condition
fails, triggers a debug-only WARNING with the failing condition as the
message, and does an early return. Kind of a poor man's exception.
PROBLEMS/ISSUES:
- Given that NS_ASSERTION is non-fatal*, its name is confusing. For
those used to libc's assert() or the JS engine's JS_ASSERT, the
non-fatalness is surprising.
* There are arguments on both sides whether NS_ASSERTION should be
fatal (makes it hard to ignore them) or non-fatal (you can keep
working if you hit one, and some of the ones we see are in old code
and we don't know if they're valid). I don't really want to get into
that right now if I can avoid it.
- ASSERTION and ERROR are used interchangeably.
- The difference between NS_ABORT and NS_RUNTIMEABORT is non-obvious.
- NS_ENSURE_* have terrible names that indicate neither their WARNING
aspect not their early-return aspect.
- stderr gets spammed with all sorts of stuff. This include warnings,
many of which are presumably unimportant (otherwise you wouldn't get
<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=341986">dozens
just starting up the browser</a>), but also all those DOMWINDOW and
DOCSHELL lines.
SUGGESTIONS
- Rename NS_ASSERTION as NS_ERROR_IF_FALSE. This would match
NS_{WARN,ABORT}_IF_FALSE and also uses "ERROR" consistently. I think
it would avoid the use of the term "assertion" altogether, which
probably isn't a bad thing.
[This partially conflicts with the "make assertions fatal" bug
(https://bugzilla.mozilla.org/show_bug.cgi?id=279923); if it
happened, making "assertions" fatal would require renaming all
NS_ERROR_IF_FALSE calls as NS_ABORT_IF_FALSE, rather than just
changing the definition of NS_ASSERTION.]
- Rename NS_ENSURE_*:
- NS_ENSURE_TRUE --> NS_WARN_AND_RETURN_IF_FALSE
- NS_ENSURE_FALSE --> NS_WARN_AND_RETURN_IF_TRUE
- NS_ENSURE_SUCCESS --> NS_WARN_AND_RETURN_IF_NOT_OK
- likewise for the more specialized ones
Note that |NS_RETURN_IF_FALSE(x, r)| is more verbose than |if (!x)
return r;|, I'd be happy if people used the return form directly.
- Encourage phasing out of NS_ENSURE_* by introducing
NS_RETURN_IF_FALSE, NS_RETURN_IF_TRUE, NS_RETURN_IF_NOT_OK.
- I'd like it if stderr only got a message if something seriously
wrong (ie. "file a bug immediately!") happened. This means:
* Don't log DOMWINDOW and DOCSHELL events by default to stderr.
They're annoying. Isn't this kind of thing supposed to be done with
NSPR logging? People who need these events can do
"NSPR_LOG_MODULE=domwindow:5" or whatever.
* Get rid of warnings. What's the point of them? They're non-fatal
just like assertions. If they're important, they should be turned
into assertions and fixed. If they're not important, why spam stderr
with them?
Obviously, these suggestions involve varying amounts of work and
tree-churn. Please tell me why they cannot be implemented :P
(Thanks to Jesse for help drafting this message; note that he doesn't
necessarily condone my suggestions.)
Nick
Kyle Huey
<n.neth...@gmail.com>wrote:
>
> THE CURRENT SITUATION
> - BREAK: causes a debugger break?
>
Yes. This is kind of a one off thing though, not for general use.
Macros that do stuff in all builds:
>
> - NS_RUNTIMEABORT (2930): triggers an ABORT with no specific message
>
This takes a message that appears in the crash report.
- NS_ENSURE_TRUE (5048), NS_ENSURE_FALSE (59), NS_ENSURE_SUCCESS
> (9637), and a bunch of other more specialized ones: if the condition
> fails, triggers a debug-only WARNING with the failing condition as the
> message, and does an early return. Kind of a poor man's exception.
>
Yes. There are many reasons we aren't using C++ exceptions.
PROBLEMS/ISSUES:
>
> * There are arguments on both sides whether NS_ASSERTION should be
> fatal (makes it hard to ignore them) or non-fatal (you can keep
> working if you hit one, and some of the ones we see are in old code
> and we don't know if they're valid). I don't really want to get into
> that right now if I can avoid it.
>
Bogus assertions should just be removed.
- ASSERTION and ERROR are used interchangeably.
>
Yes. I think NS_ERROR is mostly confined to old code.
- The difference between NS_ABORT and NS_RUNTIMEABORT is non-obvious.
>
NS_ABORT has 7 uses. We could just remove it.
- NS_ENSURE_* have terrible names that indicate neither their WARNING
> aspect not their early-return aspect.
> - stderr gets spammed with all sorts of stuff. This include warnings,
> many of which are presumably unimportant (otherwise you wouldn't get
> <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=341986">dozens
> just starting up the browser</a>), but also all those DOMWINDOW and
> DOCSHELL lines.
>
These warnings are very useful. They essentially give you a backtrace of
the exception's path by just looking at the terminal. If there are things
that are expected during normal operation they should be using if
(NS_FAILED(rv)) return rv;, etc.
> SUGGESTIONS
>
> - Rename NS_ENSURE_*:
> - NS_ENSURE_TRUE --> NS_WARN_AND_RETURN_IF_FALSE
> - NS_ENSURE_FALSE --> NS_WARN_AND_RETURN_IF_TRUE
> - NS_ENSURE_SUCCESS --> NS_WARN_AND_RETURN_IF_NOT_OK
> - likewise for the more specialized ones
>
This is substantially more typing, and this gets typed *a lot*. I also
disagree that this obscures control flow. Once you spend 5 seconds to learn
the macros it's obvious.
Note that |NS_RETURN_IF_FALSE(x, r)| is more verbose than |if (!x)
> return r;|, I'd be happy if people used the return form directly.
>
We should just not implement NS_RETURN_IF_FALSE ...
- Encourage phasing out of NS_ENSURE_* by introducing
> NS_RETURN_IF_FALSE, NS_RETURN_IF_TRUE, NS_RETURN_IF_NOT_OK.
>
The point of the NS_ENSURE macros is to warn. If you're not warning, write
the return explicitly.
- I'd like it if stderr only got a message if something seriously
> wrong (ie. "file a bug immediately!") happened. This means:
>
Why? This is an opinion, without any argument as to why it's the way to go
...
* Don't log DOMWINDOW and DOCSHELL events by default to stderr.
> They're annoying. Isn't this kind of thing supposed to be done with
> NSPR logging? People who need these events can do
> "NSPR_LOG_MODULE=domwindow:5" or whatever.
>
These are useful to get a look at our GC/CC behavior from a glance. I
wouldn't particularly object to moving them off somewhere, but I know others
will.
You're proceeding here from the assumption that any noise in the terminal is
bad. I think you should spend some time thinking about that assumption and
then attempting to convince the rest of us of that, because a lot of this
terminal output is useful in diagnosing why things are occurring the way
that they are.
- Kyle
Nicholas Nethercote
>
> You're proceeding here from the assumption that any noise in the terminal is
> bad. I think you should spend some time thinking about that assumption and
> then attempting to convince the rest of us of that, because a lot of this
> terminal output is useful in diagnosing why things are occurring the way
> that they are.
stderr output is a debased currency. It's just like compiler warnings
(outside of the JS engine) -- there are already so many you don't
notice when new ones occur.
The specific motivation for me was the crash in bug 654573, where a
warning was being printed that explained exactly what was going wrong,
but I didn't see it and so spent several hours working it out
manually. To which you'll probably say "well, that should have been
an ERROR/ASSERTION instead of a warning", to which I'll say again
"well what's the point of warnings?"
Nick
Nicholas Nethercote
Sorry, I don't buy the "it's obvious once you know it" argument.
Nick
Jonas Sicking
I agree that the current names aren't great, and that a more obvious
name would be better. But like Kyle points out, the NS_ENSURE_ macros
are used *a lot*, and probably should be used even more. A name as
long as the ones suggested would also cause readability issues and
would discourage their use.
I'd love to hear other name suggestions though.
/ Jonas
Jonas Sicking
How about:
RETURN_IF_FALSE
RETURN_IF_TRUE
RETURN_IF_ERROR
No need for NS_ :)
/ Jonas
Robert O'Callahan
> wrote:
> THE CURRENT SITUATION
>
Thanks for that summary, it's very helpful.
> - Rename NS_ASSERTION as NS_ERROR_IF_FALSE. This would match
> NS_{WARN,ABORT}_IF_FALSE and also uses "ERROR" consistently. I think
> it would avoid the use of the term "assertion" altogether, which
> probably isn't a bad thing.
> [This partially conflicts with the "make assertions fatal" bug
> (https://bugzilla.mozilla.org/show_bug.cgi?id=279923); if it
> happened, making "assertions" fatal would require renaming all
> NS_ERROR_IF_FALSE calls as NS_ABORT_IF_FALSE, rather than just
> changing the definition of NS_ASSERTION.]
>
I'd be OK with this change in principle, especially if it gets the
"assertions should be fatal" gang off my back. But NS_ERROR_IF_FALSE is
wordy to type. Could we use something else like NS_EXPECTED? And is it
really worth churning 10,000 lines of code to change the name?
Also, you'd have to change the asserts() syntax in all the reftest.lists to
be "expected()" or whatever, and change documentation, etc.
* Get rid of warnings. What's the point of them? They're non-fatal
> just like assertions. If they're important, they should be turned
> into assertions and fixed. If they're not important, why spam stderr
> with them?
>
They are sometimes useful. Maybe there should be a "warnings" PR_LOG module.
Rob
--
"Now the Bereans were of more noble character than the Thessalonians, for
they received the message with great eagerness and examined the Scriptures
every day to see if what Paul said was true." [Acts 17:11]
Neil
>- ASSERTION: prints a message to stderr, is detected by fuzzers, causes test failures for some test suites. Pops up a dialog on Windows?
>
>
Indeed, and the dialog is customisable so that you can make some
assertions more fatal than others.
>- ASSERTION and ERROR are used interchangeably.
>
>
I hope not; ERROR is unconditional, and it doesn't make sense to write
NS_ASSERTION(PR_FALSE, ...);
>[This partially conflicts with the "make assertions fatal" bug (https://bugzilla.mozilla.org/show_bug.cgi?id=279923); if it happened, making "assertions" fatal would require renaming all NS_ERROR_IF_FALSE calls as NS_ABORT_IF_FALSE, rather than just changing the definition of NS_ASSERTION.]
>
I'm not sure what bug 279923 is intending but NS_ABORT_IF_FALSE only
applies to debug builds. The irony is that I ignore (see above) the
assertion for dereferencing a null nsCOMPtr with operator->() because
it's easier to debug the ensuing crash directly.
--
Warning: May contain traces of nuts.
Robert Kaiser
> - Given that NS_ASSERTION is non-fatal*
I thought they were made fatal in debug builds. Was that planned but
never done, or reverted?
Robert Kaiser
--
Note that any statements of mine - no matter how passionate - are never
meant to be offensive but very often as food for thought or possible
arguments that we as a community should think about. And most of the
time, I even appreciate irony and fun! :)
Benjamin Smedberg
> On Thu, Jun 9, 2011 at 10:01 PM, Nicholas Nethercote
> <n.neth...@gmail.com> wrote:
>> On Fri, Jun 10, 2011 at 2:44 PM, Kyle Huey<m...@kylehuey.com> wrote:
>>> Once you spend 5 seconds to learn the macros it's obvious.
>> Sorry, I don't buy the "it's obvious once you know it" argument.
> I agree that the current names aren't great, and that a more obvious
> name would be better. But like Kyle points out, the NS_ENSURE_ macros
> are used *a lot*, and probably should be used even more. A name as
> long as the ones suggested would also cause readability issues and
> would discourage their use.
>
> I'd love to hear other name suggestions though.
codebase. I forbid the use of the NS_ENSURE_* macros in my modules and
code I review and strongly discourage their use in general. They make
the code harder for new contributors to read, obscuring the control flow
and requiring people to refer to nsDebug.h all the time. I remember my
first two years in the codebase were spent with nsDebug.h open in MXR
constantly as I figured out what all these damn macros did.
I would much prefer the slightly more verbose version:
if (!cond) {
NS_WARNING("Unexpected condition"); // or NS_ASSERTION
return ERROR_CODE;
}
--BDS
Nicholas Nethercote
<benj...@smedbergs.us> wrote:
>
> I agree with Nick that the NS_ENSURE_* macros are a blight on our codebase.
>
>
> if (!cond) {
> NS_WARNING("Unexpected condition"); // or NS_ASSERTION
> return ERROR_CODE;
> }
I sympathize! But the reason I suggested renaming to
NS_WARN_AND_RETURN_IF_{TRUE,FALSE,NOT_OK} is that it only requires a
simple search-and-replace change, whereas rewriting to your version is
much harder.
Nick
Nicholas Nethercote
> Nicholas Nethercote schrieb:
>>
>> - Given that NS_ASSERTION is non-fatal*
>
> I thought they were made fatal in debug builds. Was that planned but never
> done, or reverted?
Never done, it seems. See
https://bugzilla.mozilla.org/show_bug.cgi?id=279923. JS_ASSERT in the
JS engine is fatal, however.
Nick
Benjamin Smedberg
indentation correct. Assuming we (as a project, not just my modules)
decide that the NS_ENSURE_* are bad and should be gotten rid of, we can
invest a little time and actually make our code better.
--BDS
L. David Baron
> Nicholas Nethercote schrieb:
> >- Given that NS_ASSERTION is non-fatal*
>
> I thought they were made fatal in debug builds. Was that planned but
> never done, or reverted?
But there's still work to work our way in that direction; the next
step is https://bugzilla.mozilla.org/show_bug.cgi?id=404077 , which
would do the same for mochitest as I've done for reftest. Then we'd
be much likely to stop regressing our ASSERTION counts, which will
allow us to make progress on fixing them. I should probably try to
make further progress there, though I was somewhat stuck due to bug
631289.
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
Joshua Cranmer
> They are sometimes useful. Maybe there should be a "warnings" PR_LOG
> module.
Perhaps it would be more useful if
a) warnings could also show up in the error console
b) warnings were enabled on nightly (/aurora?) builds as well so that it
might be useful information to extension developers.
Joshua Cranmer
> I sympathize! But the reason I suggested renaming to
> NS_WARN_AND_RETURN_IF_{TRUE,FALSE,NOT_OK} is that it only requires a
> simple search-and-replace change, whereas rewriting to your version is
> much harder.
It would take probably only a weekend to write an automated rewriting
tool that rewrote macro invocations.
Jonas Sicking
> On Friday 2011-06-10 15:44 +0200, Robert Kaiser wrote:
>> >- Given that NS_ASSERTION is non-fatal*
>>
>> I thought they were made fatal in debug builds. Was that planned but
>> never done, or reverted?
>
> step is https://bugzilla.mozilla.org/show_bug.cgi?id=404077 , which
> would do the same for mochitest as I've done for reftest. Then we'd
> be much likely to stop regressing our ASSERTION counts, which will
> allow us to make progress on fixing them. I should probably try to
> make further progress there, though I was somewhat stuck due to bug
> 631289.
I think we should assume that we'll end up being successful eventually
in this project. It certainly would help us make a better product.
/ Jonas
Ehsan Akhgari
>> - Rename NS_ASSERTION as NS_ERROR_IF_FALSE. This would match
>> NS_{WARN,ABORT}_IF_FALSE and also uses "ERROR" consistently. I think
>> it would avoid the use of the term "assertion" altogether, which
>> probably isn't a bad thing.
>> (https://bugzilla.mozilla.org/show_bug.cgi?id=279923); if it
>> happened, making "assertions" fatal would require renaming all
>> NS_ERROR_IF_FALSE calls as NS_ABORT_IF_FALSE, rather than just
>> changing the definition of NS_ASSERTION.]
>>
>
> "assertions should be fatal" gang off my back. But NS_ERROR_IF_FALSE is
> wordy to type. Could we use something else like NS_EXPECTED? And is it
> really worth churning 10,000 lines of code to change the name?
I'm very much against any renaming of these macros. Here's why:
NS_RETURN_IF_FAILURE (or whatever other name we come up with) is not
much clearer than NS_ENSURE_SUCCESS. Try to pretend that you're a
newcomer to our code base, and read this code:
nsresult rv = editor->DoSomethingForMe(presShell, getter_AddRefs(obj));
NS_RETURN_IF_FAILURE(rv, rv);
Is it really obvious what this code does without looking at the
definition of the macro? Why does it take two parameters which are both
|rv| here? If you're smart enough to figure out that one of them is
used for the condition check and the other for return value, which one
is which?
I do, however, think that we should rewrite these macros to just print a
warning and return explicitly. I've been trained to ignore this
ugliness in my day-to-day work (and I do use these macros in my own
code), but macros hiding control flow structure are evil in my opinion.
If somebody writes a tool to do this rewrite, I'd take a patch for
editor/ in a heartbeat.
> * Get rid of warnings. What's the point of them? They're non-fatal
>> just like assertions. If they're important, they should be turned
>> into assertions and fixed. If they're not important, why spam stderr
>> with them?
>>
>
> They are sometimes useful. Maybe there should be a "warnings" PR_LOG module.
I think the main problem is that every one of those warnings are not
useful for all developers. It just takes some time for your eyes to get
trained on what to look for and what to ignore. I don't think that
trying to print very little to stderr is actually a good thing. I don't
buy the argument that only things which should cause you to file a bug
should go to stderr. We have better tools to deal with those types of
things (assertions/aborts/etc).
Cheers,
Ehsan
Steve Fink
I'll repeat my suggestion from the blog comment thread, since iiuc it's
exactly what you are saying: how about changing
NS_ENSURE_TRUE(condition);
to
if (NS_WARN_UNLESS(condition))
return NS_ERROR_KABOOM;
and
NS_ENSURE_FALSE(false-condition);
to
if (NS_WARN_IF(false-condition))
return NS_ERROR_YOUSUCK;
?
It makes both the warningness and the early-return explicit. It's about
the same amount of typing. It's a little tricky for an automated
rewriter because of the dangling-if. It still spams stderr, so something
like
if (NS_WARN_UNLESS(condition, channel))
would be better, but that's way harder to do mechanically. (Though if
channel could be inferred from the source filename via a set of rules,
and the build system constructed a header file of channel #defines from
the same set of rules, a reasonable initial value *could* be automated.
Then again, you may as well do the single-argument renaming first, and
add the channel version in a separate pass.)
The main thing I like about this is that it makes it much easier to see
on a later visual inspection whether an individual call could be
rewritten from
if (NS_WARN_UNLESS(condition))
return NS_ERROR_KABOOM;
to
if (condition)
return NS_ERROR_KABOOM;
or
NS_WARN_UNLESS(condition);
/* no return */
ie, when someone cargo-culted NS_ENSURE_* without realizing either the
warning nature or the early-return nature. Hopefully the latter doesn't
apply very often.
I recognize that this doesn't directly handle the variants. But where a
variant is only used to set a return value (eg NS_ENSURE_ARG), I much
prefer the explicit
if (NS_WARN_UNLESS(arg))
return NS_ERROR_INVALID_ARG;
because I have been burned by the return values being hidden. (I got an
error. I grepped through all relevant source for the specific return
value and found a couple, all of which were red herrings because the
real trigger was hidden in a macro.)
The NS_ENSURE_* variants that do specialized conditions would be
converted to NS_WARN_IF/UNLESS_* variants.
The exact names, of course, are open to debate. (Of both productive and
bikeshed varieties.)
Jonas Sicking
> in a heartbeat.
I'm not a fan of changing these macros to explicit code. I'm all for
removing macros, however I think this is an example when they are
actually making the code more readable.
We currently have a ton of code which looks like the following:
rv = foo->DoSomething(getter_AddRefs(bar));
NS_ENSURE_SUCCESS(rv, rv);
rv = bar->BeUseful(42);
NS_ENSURE_SUCCESS(rv, rv);
This is the code density here is effectively 50%, definitely making
the code harder to read. If we were forced to turn it into:
rv = foo->DoSomething(getter_AddRefs(bar));
if (NS_FAILED(rv)) {
NS_WARNING(rv);
return rv;
}
rv = bar->BeUseful(42);
if (NS_FAILED(rv)) {
NS_WARNING(rv);
return rv;
}
Now we're down to an abysmal 20% which is really terrible. Ideally we
should of course change a lot of functions to not return nsresults,
but that's something that we've been working on for years, and we'll
likely be working on for years to come.
/ Jonas
Ehsan Akhgari
> exactly what you are saying
I think this is a very good proposal, except for the channel part, as
I'm still not convinced that we should not use stderr as an integration
point for all these types of warnings.
Cheers,
Ehsan
Marco Bonardo
> The main thing I like about this is that it makes it much easier to see
> on a later visual inspection whether an individual call could be
> rewritten from
>
> if (NS_WARN_UNLESS(condition))
> return NS_ERROR_KABOOM;
>
> to
>
> if (condition)
> return NS_ERROR_KABOOM;
>
> or
>
> NS_WARN_UNLESS(condition);
> /* no return */
FWIW, we have NS_WARN_IF_FALSE that may clash with this. So I'm
suggesting, that in case we do this rename, we unify the 2
warning-on-condition macros.
-m
Ehsan Akhgari
>> in a heartbeat.
>
> I'm not a fan of changing these macros to explicit code. I'm all for
> removing macros, however I think this is an example when they are
> actually making the code more readable.
But it can also cause us to get better code by having smaller functions
because of this reason. :-)
Still, I see control flow statements being hidden in macros is a bad
thing. I know that I've sometimes failed to note the return in these
macros myself, even though I'm not exactly a newcomer, and my eyes
almost treat NS_ENSURE_SUCCESS as a keyword now... :-)
Cheers,
Ehsan
Jonas Sicking
I agree that they're a bad thing. But I think they are a less bad
thing than the resulting unreadable code demonstrated above.
I do think that it would help to have 'return' in the macro name
though as that would make it easier to see when searching.
/ Jonas
smaug
Hmm, actually, different NS_ENSURE_*
macros are being used in a bit different way.
- When NS_ENSURE_TRUE is used, it is pretty clear what
you're going to return, since that needs to be specified as a
latter parameter.
- NS_ENSURE_SUCCESS is often used like
"just-do-some-check-to-the-rv-value".
And since both the parameters are usually just rv,
it may not be quite clear what the macro will actually do.
- NS_ENSURE_STATE is kind of runtime precondition
(and I tend to use it quite a lot since it gives the
very useful warnings). The name of the macro is pretty clear,
and it happens to be too useful to get rid of.
- (there are more NS_ENSURE_* macros)
So, perhaps we could do something to NS_ENSURE_SUCCESS,
like remove it. If the warning is needed, one could always write
NS_ENSURE_TRUE(NS_SUCCEEDED(rv), rv);
but usually code would be just
if (NS_FAILED(rv)) {
return rv;
}
-Olli
Steve Fink
rv = foo->DoSomething(getter_AddRefs(bar));
if (NS_WARN_UNLESS(rv))
return rv;
rv = bar->BeUseful(42);
if (NS_WARN_UNLESS(rv))
return rv;
Down to 33%, but the returns pop out now. I'd say that's worth it.
----
Independent of NS_ENSURE_SUCCESS vs NS_WARN_UNLESS:
The above makes the silliness of the above code a little more apparent
-- you're asking it to warn on failure, but the only information you're
giving it is rv. Which really isn't enough for a meaningful message, and
this probably ought to just be
if (!(rv = foo->DoSomething(getter_AddRefs(bar))))
return rv;
if (!(rv = bar->BeUseful(42)))
return rv;
Now you're back up to 50% and you've suppressed a message that looks like
NS_ENSURE_SUCCESS(rv,rv) failed with result 0xdeadbeef
which has too much useless fluff. If there's really some use in having
that message, then either (a) you should be typing in a meaningful
message, in which case you can't use NS_ENSURE_SUCCESS; or (b) you
should be passing eg foo->DoSomething(...) to the macro so it can use it
in the message.
Sure, the way it is now (or with the proposed NS_WARN_UNLESS) you get a
noisy audit trail leading back from Something Bad Happening, but imho
the cost of making stderr useless is too high.
On the other hand, that sort of excessively verbose trail is fine for a
log file. So if you instead decide to switch it to
if (NS_LOG_UNLESS(rv))
return rv;
then the cost/benefit is more favorable, though that log file would
still be largely useless to me personally.
Speaking as a relative newbie, the current stderr output is meaningless.
There are so many things that look like problems in the *successful*
output that it's nearly impossible to discover whether it's trying to
tell me something related to the problem I'm experiencing. And even when
I do manage to identify the relevant spew, it makes me work to decode
weird hex values to figure out what it's attempting to say.
For people who are deeply familiar with the current way of doing things,
I think it's really hard to grasp just how unapproachable and hostile
this makes our system. stderr spew should be a gentle introduction to
finding and resolving your own problems, by providing a crumb here and
there that with grep, Google, and comparative runs on different input,
you can find your way into the relevant code. The current firehose makes
that path a non-starter.
THE SKY IS FALLING THE SKY IS FALLING THE SKY IS FALLING THE SKY IS
FALLING there's an acorn rolling around by your foot THE SKY IS FALLING
THE SKY IS FALLING THE SKY IS FALLING THE SKY IS FALLING THE SKY IS FALLING
Steve Fink
rv = foo->DoSomething(getter_AddRefs(bar));
if (NS_WARN_UNLESS(rv))
return rv;
rv = bar->BeUseful(42);
Joshua Cranmer
> - NS_ENSURE_SUCCESS is often used like
> "just-do-some-check-to-the-rv-value".
> And since both the parameters are usually just rv,
> it may not be quite clear what the macro will actually do.
It seems the biggest beef people have is that NS_ENSURE_SUCCESS(rv, rv);
is too confusing for new people. Some notes I have:
1. I've used some modules where if (NS_FAILED(rv)) return rv; is used...
and the end result is that I now have to run back into my debugger and
try to catch the exact case to figure out where this new error I'm
getting was thrown from. Although it still doesn't quite help 100% of
the time since it doesn't go all the way back to the actual first return.
2. The main problem we have is that we basically use rv as our fake
exception framework. This basically means that we have two issues with
it: we always have to check it after every method, and we want to be
able to get some sort of pseudo-stack trace if it fails.
3. I'm not terribly annoyed to have to write the if/return, especially
since it fits nicely on one line:
NS_ENSURE_SUCCESS(rv, rv);
if (NS_FAILED(rv)) return rv;
~~~ <-- 3 characters difference.
Yes, the latter form is arguably a contravention of our style guide, but
I wouldn't feel bad making a special case for one of the most heavily
used patterns in the codebase. It should also hopefully be pretty
obvious to any onlookers at that point that rv is a poor man's exception.
The following is a _very_ wacky idea:
Since Mozilla 2.0, we are no longer bound to keep ABI compatibility of
core XPCOM code the same. What if, instead of
typedef PRUint32 nsresult;
we made the following:
struct nsresult {
PRUint32 code;
nsresult(PRUint32 init) : code(init) {
#ifdef DEBUG
printf("Hey, you got an nsresult exception!\n");
DumpStack();
#endif
}
nsresult(nsresult other) : code(other.code) {}
operator PRUint32() { return code; }
};
(this would obviously require changes to other code).
Jason Duell
> rv = foo->DoSomething(getter_AddRefs(bar));
> if (NS_WARN_UNLESS(rv))
> return rv;
> rv = bar->BeUseful(42);
> if (NS_WARN_UNLESS(rv))
> return rv;
>
> Down to 33%, but the returns pop out now. I'd say that's worth it.
+1 on general concept of busting out the return statement but keeping
the warning mechanics brief.
Note we're talking about nsresults here, so even the concept of "true
or false" is non-intuitive to novices (success doesn't always equal
0). So perhaps we need
rv = foo->DoSomething(getter_AddRefs(bar));
if (WARN_IF_NS_FAILED(rv))
return rv;
If we changed NS_ASSERTION and friends to evaluate to to true/false,
they could be used the same way for when we wish to do something
besides printing to stderr.
Jason
Jason
L. David Baron
> If we changed NS_ASSERTION and friends to evaluate to to true/false,
> they could be used the same way for when we wish to do something
> besides printing to stderr.
We don't want to do that, since we depend (for performance) on
NS_ASSERTION *not* evaluating its argument in non-DEBUG builds.
Justin Dolske
> I'm very much against any renaming of these macros. Here's why:
> NS_RETURN_IF_FAILURE (or whatever other name we come up with) is not
> much clearer than NS_ENSURE_SUCCESS. Try to pretend that you're a
> newcomer to our code base, and read this code:
>
> nsresult rv = editor->DoSomethingForMe(presShell, getter_AddRefs(obj));
> NS_RETURN_IF_FAILURE(rv, rv);
Well, in the grand scheme of things, I think there are far more
troublesome things in our codebase for newcomers. *cough*xpcom*cough* No
matter what we call this macro (or if we de-macro it), we're talking
about a very modest change to the readability of the code.
The pattern of "x = foo(); check x for failure" is so pervasive in
programming that I think people will quickly catch on no matter how
obfuscated it is.
I think there's value in renaming the macros so it's clear that there's
an implicit return. [I always thought the "ensure success" was odd
naming, as if you're making an after-the-fact prayer that nothing went
wrong. ;-)]
> Why does it take two parameters which are both |rv| here?
Yeah, I've never liked that either. NS_ENSURE_SUCCESS(rv, NS_BLAH) is
something I've always tried to avoid. Too magical.
> If somebody writes a tool to do this rewrite, I'd take a patch for
> editor/ in a heartbeat.
Clever trick, getting more people to have blame for editor/ code. ;-)
Justin
Shawn Wilsher
> 1. I've used some modules where if (NS_FAILED(rv)) return rv; is used...
> and the end result is that I now have to run back into my debugger and
> try to catch the exact case to figure out where this new error I'm
> getting was thrown from. Although it still doesn't quite help 100% of
> the time since it doesn't go all the way back to the actual first return.
debugger means I have to reproduce it too, which I can't always do.
Cheers,
Shawn
Neil
> I would much prefer the slightly more verbose version:
>
> if (!cond) {
> NS_WARNING("Unexpected condition"); // or NS_ASSERTION
> return ERROR_CODE;
> }
Perhaps
#ifdef DEBUG
inline boolean NS_Warn_boolean(boolean cond, const char *aStr, const
char *aExpr, const char *aFile, PRInt32 aLine)
{
if (!cond)
NS_DebugBreak(NS_DEBUG_WARNING, aStr, nsnull, aFile, aLine);
return cond;
}
inline boolean NS_Warn_nsresult(nsresult rv, const char *aStr, const
char *aExpr, const char *aFile, PRInt32 aLine)
{
if (NS_FAILED(rv)) {
char *msg = PR_smprintf("NS_SUCCEEDED(%s) failed with result
0x%X", aStr, rv);
NS_DebugBreak(NS_DEBUG_WARNING, msg, nsnull, aFile, aLine);
PR_smprintf_free(msg);
}
return NS_FAILED(rv);
}
#define NS_NOISY_FALSE(cond) NS_Warn_boolean(!(cond), #cond, __FILE__,
__LINE__)
#define NS_NOISY_FAILED(rv) NS_Warn_nsresult(NS_SUCCEEDED(rv), #rv,
__FILE, __LINE)
#else
#define NS_NOISY_FALSE(cond) (!(cond))
#define NS_NOISY_FAILED(rv) NS_FAILED(rv)
#endif
Then you can write
if (NS_NOISY_FAILED(rv))
return rv;
[I had also considered calling them NS_WARN_IF_FALSE and NS_WARN_IF_FAILED]
Shawn Wilsher
> The above makes the silliness of the above code a little more apparent
> -- you're asking it to warn on failure, but the only information you're
> giving it is rv. Which really isn't enough for a meaningful message, and
> this probably ought to just be
directly though).
> Sure, the way it is now (or with the proposed NS_WARN_UNLESS) you get a
> noisy audit trail leading back from Something Bad Happening, but imho
> the cost of making stderr useless is too high.
People should *not* be using this macro in cases where failure is
expected, which means it should only happen when you are changing code
and you break something. At which point, that audit trail becomes very
handy because you can track the failure down without firing up a debugger.
Cheers,
Shawn
johnjbarton
>At which point, that audit trail becomes very
> handy because you can track the failure down without firing up a debugger.
As a development tools person, I'm curious about this comment. Is the
cost of starting the debugger really high? Is there a reason you do not
routinely run from within the debugger rather than start it after you
find a problem?
jjb
Jonas Sicking
I feel like more and more people are advocating this pattern. I think
it's something I could live with. Though here's a IMHO more readable
syntax:
if (warn_failed(rv))
return rv;
if (warn_false(cond))
return NS_ERROR_UNEXPECTED;
if (warn_true(cond))
return NS_ERROR_FIDDLESTICKS;
/ Jonas
Joshua Cranmer
When I'm developing stuff, I don't routinely open up Thunderbird in a
debugger because, well, it's annoying and I don't need it most of the
time. As Shawn pointed out earlier, sometimes you hit problems that you
can't reproduce. And debuggers would help less than you suspect because
debuggers require you to actively search out and declare "I think this
is where the problem happens" via breakpoints, while the NS_ENSURE_*
macros passively leave a trail of when things fail, so they can find
problems you didn't know you had.
Boris Zbarsky
> On 6/12/2011 8:51 AM, Shawn Wilsher wrote:
>> At which point, that audit trail becomes very
>> handy because you can track the failure down without firing up a
>> debugger.
>
> As a development tools person, I'm curious about this comment. Is the
> cost of starting the debugger really high?
It can be, yes, depending on your OS and other settings (though this has
gotten better recently). And running under the debugger can be much
slower than running not under a debugger...
Also, and running our tests under a debugger is a trail of tears because
people keep breaking the --debugger command-line option.
But more importantly, for bugs that are not reliably reproducible you
may have seen it not under a debugger once (and have a debug tinderbox
log for that!)... and then you can't reproduce it anymore.
-Boris
Henri Sivonen
> I'll repeat my suggestion from the blog comment thread, since iiuc it's
>
> NS_ENSURE_TRUE(condition);
>
> to
>
> if (NS_WARN_UNLESS(condition))
> return NS_ERROR_KABOOM;
I think the NS_ENSURE_* macros are useful and aren't *that* hard for
someone new to learn. (In fact, I think they are the least of worries
when learning C++ Mozillaisms.)
However, I think it's a bug that NS_ENSURE_TRUE warns. Having it warn
makes it less useful for cases where the early return happens in a
legitimate situation and you don't want to spam the console.
--
Henri Sivonen
hsiv...@iki.fi
http://hsivonen.iki.fi/
Shawn Wilsher
> However, I think it's a bug that NS_ENSURE_TRUE warns. Having it warn
> makes it less useful for cases where the early return happens in a
> legitimate situation and you don't want to spam the console.
case, people *should not* be using the macros. That's been the rule of
thumb for as long as I've been around, and I've certainly seen reviewers
enforce that.
Cheers,
Shawn
Jonas Sicking
For what it's worth, if we changed all current macros to something like:
if (warn_false(cond))
return NS_ERROR_FIDDLESTICKS;
it would both be easier to see that it warns, and to change it to not warn:
if (!cond)
return NS_ERROR_FIDDLESTICKS;
/ Jonas
Ehsan Akhgari
> Since Mozilla 2.0, we are no longer bound to keep ABI compatibility of
> core XPCOM code the same. What if, instead of
> typedef PRUint32 nsresult;
>
> we made the following:
>
> struct nsresult {
> PRUint32 code;
> nsresult(PRUint32 init) : code(init) {
> #ifdef DEBUG
> printf("Hey, you got an nsresult exception!\n");
> DumpStack();
> #endif
> }
> nsresult(nsresult other) : code(other.code) {}
> operator PRUint32() { return code; }
> };
I think this is a really good idea!
Has anybody experimented with this to see how much code we need to
change in order to make this work?
Ehsan
Benjamin Smedberg
> On 11-06-10 7:24 PM, Joshua Cranmer wrote:
>> core XPCOM code the same. What if, instead of
>> typedef PRUint32 nsresult;
>>
>> we made the following:
>>
>> struct nsresult {
>> PRUint32 code;
>> nsresult(PRUint32 init) : code(init) {
>> #ifdef DEBUG
>> printf("Hey, you got an nsresult exception!\n");
>> DumpStack();
>> #endif
>> }
>> nsresult(nsresult other) : code(other.code) {}
>> operator PRUint32() { return code; }
>> };
>
nsresult. How would you distinguish the normal cases from the truly
unusual ones?
--BDS
Ehsan Akhgari
> On 6/13/2011 10:04 AM, Ehsan Akhgari wrote:
>> On 11-06-10 7:24 PM, Joshua Cranmer wrote:
>>> core XPCOM code the same. What if, instead of
>>> typedef PRUint32 nsresult;
>>>
>>> we made the following:
>>>
>>> struct nsresult {
>>> PRUint32 code;
>>> nsresult(PRUint32 init) : code(init) {
>>> #ifdef DEBUG
>>> printf("Hey, you got an nsresult exception!\n");
>>> DumpStack();
>>> #endif
>>> }
>>> nsresult(nsresult other) : code(other.code) {}
>>> operator PRUint32() { return code; }
>>> };
>>
> Why? There are many "normal" cases where we return/throw an error code
> nsresult. How would you distinguish the normal cases from the truly
> unusual ones?
We could use the type system to detect such cases. for example:
struct dont_warn {
dont_warn(PRUint32 code) : code(code) {}
PRUint32 code;
};
// inside nsresult:
nsresult(dont_warn wrapper) : code(wrapper.code) {}
// expected failure case:
rv = dont_warn(NS_ERROR_FAILURE);
But anyways, this is all wishful thinking, since the resulting type
would be non-POD, which means that it can't be passed to varargs
functions (among other problems).
Cheers,
Ehsan
Robert O'Callahan
> Has anybody experimented with this to see how much code we need to change
> in order to make this work?
>
Before trying anything like this, you would want to very carefully check
that in opt builds with our supported compilers it generates exactly the
same code (nsresults still passed/returned in registers, etc).
Rob
--
"Now the Bereans were of more noble character than the Thessalonians, for
they received the message with great eagerness and examined the Scriptures
every day to see if what Paul said was true." [Acts 17:11]
Nicholas Nethercote
In an attempt to extract some useful actions out of this discussion,
I've posted two follow-up threads with some smaller, specific
proposals:
- http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/fb8e621365929dea#
- http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/390bf89020acc020#
These focus on NS_ENSURE_* and warnings. Other changes I originally
suggested clearly don't have much interest from others.
Thanks for the discussion, it's been enlightening!
Nick
Mook
> expected, which means it should only happen when you are changing code
> and you break something. At which point, that audit trail becomes very
> handy because you can track the failure down without firing up a debugger.
Unfortunately, since this isn't a real C++ exception, you never have
enough context.
NS_IMETHODIMP foo() {
if (NS_FAILED(bar()))
return NS_OK; // expected
...
}
NS_IMETHODIMP bar() {
...
rv = baz();
NS_ENSURE_SUCCESS(rv, rv); // wasn't expecting that but the caller was
...
}
On the other hand, if bar() swallows the warning, then when quux() calls
it, expecting it to never fail, you don't get the fake traceback...
--
Mook
Jesse Ruderman
That actually looks kind of ok :)
Renaming NS_ENSURE_SUCCESS to BAIL_ON_FAIL would be amusing, though.