Banning NS_ENSURE_* macros in new code

[Benjamin Smedberg]

I've long been bothered by the NS_ENSURE_* macros.

* They hide return statements
* The issues warnings when failing, which is an unexpected behavior for
coders new to our codebase

I'd like to forbid the use of NS_ENSURE_* macros in new code. If you want a
warning, you should write an explicit warning block:

if (NS_FAILED(rv)) {
NS_WARNING("Something bad is about to happen!");
return rv;
}

otherwise just write:

if (NS_FAILED(rv))
return rv;

Thoughts? I know a couple people really like the NS_ENSURE_* macros, but I
think that overall they are very mozilla-specific and aren't good for making
our code more standard c++.

--BDS

[David Bolter]

Seconded. I'd much rather see the explicit return and optional explicit
warnings.

cheers,
David

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Mike Kristoffersen]

Benjamin Smedberg wrote:
> I've long been bothered by the NS_ENSURE_* macros.
>
> * They hide return statements
> * The issues warnings when failing, which is an unexpected behavior for
> coders new to our codebase
>
> I'd like to forbid the use of NS_ENSURE_* macros in new code. If you want a
> warning, you should write an explicit warning block:

Great suggestion! I 100 % Agree :)

^__^
MikeK

[Joshua Cranmer]

On 09/30/2009 11:31 AM, Benjamin Smedberg wrote:
> Thoughts? I know a couple people really like the NS_ENSURE_* macros, but I
> think that overall they are very mozilla-specific and aren't good for making
> our code more standard c++.

It sounds like I'm going to be in the minority here, but I kind of like
them.

My rationale:
* Style guides (last I check) recommend checking the rv after nearly all
method calls; NS_ENSURE_SUCCESS(rv, rv); is a single line, unlike
if (NS_FAILED(rv))
{
NS_WARNING("rv failed!");
return rv;
}
* If the success fails, you also get a warning, which includes the
numeric error code. This generally makes it much faster to pinpoint
which check is causing the failure, especially if someone lower in the
stack frame decides to swallow the error without telling you.

Sure, it might not be standard C++, but my knowledge of the standard C++
way is to use exceptions. So until we can use C++ exceptions, I'd rather
keep using the NS_ENSURE_* macros.

[Shawn Wilsher]

On 9/30/09 11:52 AM, Joshua Cranmer wrote:
> * If the success fails, you also get a warning, which includes the
> numeric error code. This generally makes it much faster to pinpoint
> which check is causing the failure, especially if someone lower in the
> stack frame decides to swallow the error without telling you.

+1
It means I don't have to fire up a debugger to figure out why something
is failing, and that's a huge win for me.

Cheers,

Shawn

[Benjamin Smedberg]

On 9/30/09 2:52 PM, Joshua Cranmer wrote:

> * If the success fails, you also get a warning, which includes the
> numeric error code. This generally makes it much faster to pinpoint
> which check is causing the failure, especially if someone lower in the
> stack frame decides to swallow the error without telling you.
>
> Sure, it might not be standard C++, but my knowledge of the standard C++
> way is to use exceptions. So until we can use C++ exceptions, I'd rather
> keep using the NS_ENSURE_* macros.

The problem is that many of the times we use NS_ENSURE_* failures are
perfectly normal, and you end up with unnessary or confusing cdebug spew.
NS_ENSURE_* only works for the cases where failures shouldn't happen in
normal operation, but an assertion is in appropriate because failures could
happen in some weird case, which seems like a vanishingly small problem.

As for the specific warning case, I'd be ok with making
NS_ENSURE_SUCCESS_BODY a standalone macro:

if (NS_FAILED(rv)) {
NS_WARNING_RV(rv, "Some failure"));
return rv;
}

But I still think this should be a pretty uncommon occurence. I'm
specifically concerned about the complexity of hiding returns within macros.

--BDS

[Mike Kristoffersen]

Benjamin Smedberg wrote:
> As for the specific warning case, I'd be ok with making
> NS_ENSURE_SUCCESS_BODY a standalone macro:
>
> if (NS_FAILED(rv)) {
> NS_WARNING_RV(rv, "Some failure"));
> return rv;
> }
>
> But I still think this should be a pretty uncommon occurence. I'm
> specifically concerned about the complexity of hiding returns within macros.
>
> --BDS

_If_ we absolutely want something like that (I'm still against it),
could we at least name it something that tells what it does - like
NS_RETURN_IF_ERROR, NS_RETURN_ERROR_ON_FAILURE, ... to minimize the
"hiding" of the return statement.

^__^
MikeK

[Mike Shaver]

On Wed, Sep 30, 2009 at 12:44 PM, Mike Kristoffersen
<mkristo...@mozilla.com> wrote:
> _If_ we absolutely want something like that (I'm still against it), could we
> at least name it something that tells what it does - like
> NS_RETURN_IF_ERROR, NS_RETURN_ERROR_ON_FAILURE, ...  to minimize the
> "hiding" of the return statement.

Why didn't we think of that naming in 1998?

Mike

[Shawn Wilsher]

On 9/30/09 12:14 PM, Benjamin Smedberg wrote:
> As for the specific warning case, I'd be ok with making
> NS_ENSURE_SUCCESS_BODY a standalone macro:

What about mass renaming NS_ENSURE_* to something that indicates that it
returns on failure? We can still discourage it's use in cases you've
said it's bad to use, but then we can still use it in places that the
macro is helpful.

Cheers,

Shawn

[Justin Dolske]

On 9/30/09 12:44 PM, Mike Kristoffersen wrote:

> _If_ we absolutely want something like that (I'm still against it),
> could we at least name it something that tells what it does - like
> NS_RETURN_IF_ERROR, NS_RETURN_ERROR_ON_FAILURE, ... to minimize the
> "hiding" of the return statement.

Ooh, that's a great idea!

I'd hate to lose NS_ENSURE_SUCCESS(rv,rv), for exactly the reasons
Joshua noted. But renaming it to just NS_RETURN_IF_ERROR(rv) would be a
worthy improvement.

As for the other NS_ENSURE_* macros... I'd be open to banning them,
since they're much less commonly used (at least in the code I'm familiar
with). Ditto for patterns like NS_ENSURE_SUCCESS(rv,NS_OK).

Justin

[Mike Kristoffersen]

Sounds like a nice compromise to me :) I foresee some interesting
discussions on fundamental function design (good thing, as one tend to
learn from those).

^__^
MikeK

[Mark Banner]

On 30/09/2009 22:48, Justin Dolske wrote:
> As for the other NS_ENSURE_* macros... I'd be open to banning them,
> since they're much less commonly used (at least in the code I'm familiar
> with). Ditto for patterns like NS_ENSURE_SUCCESS(rv,NS_OK).

Assuming you're just saying to ban a pattern like NS_ENSURE_SUCCESS(rv,
NS_OK) - I'm ok with that, but NS_ENSURE_SUCCESS(rv, <some other
non-nsresult return type>) can be useful as it gives the short hand for
this failed, return some other value from this non-xpcom function.

I also think that NS_ENSURE_ARG_POINTER is very useful as it is a check
that is frequently needed due to xpcom.

Standard8

[Neil]

Shawn Wilsher wrote:

It's its use in code that needs to be ported to older branches that
worries me...

Maybe we should just introduce new macros NS_ASSERT_IF_FAILED(rv),
NS_WARN_IF_FAILED(rv) and NS_PROPAGATE_FAILURE(rv) to eventually replace
NS_ENSURE_SUCCESS(rv, rv), so that it's clear how "bad" the failure
actually is. [In a non-XPCOM method, you would use e.g.
NS_WARN_IF_FAILED(rv); if (NS_FAILED(rv)) return; ]

--
Warning: May contain traces of nuts.

[Smaug]

I use NS_ENSURE_STATE all the time. It returns, IMO, the right error
code and is very simple to use. And I do tend to use it when the
error is really unexpected, which is why returning NS_ERROR_UNEXPECTED
and giving a warning is useful.

-Olli

[dbradley]

On Sep 30, 3:14 pm, Benjamin Smedberg <benja...@smedbergs.us> wrote:
> perfectly normal, and you end up with unnessary or confusing cdebug spew.
> NS_ENSURE_* only works for the cases where failures shouldn't happen in
> normal operation, but an assertion is in appropriate because failures could
> happen in some weird case, which seems like a vanishingly small problem.

Isn't that the problem, though, people blindly using them? I've always
used NS_ENSURE_ for things that were more exceptional. If there's an
error that I expect, then I do:

if (rv == NS_ERROR_SOMETHING) {
// Handle the error and return
}
NS_ENSURE_SUCCESS(rv, rv);

Honestly I've always lived by the single entry single exit and wasn't
real fond of them. But I view them as a poor man's exception
mechanism, and have used them as I would use exceptions and used them
as such.

As mentioned others pointed out, I've found them very helpful in
tracking down problems without having to go into a debugger, try and
reproduce the problem and figure out what's erroring.

Maybe we need a better alternative, but I don't think that's going to
be easy since XPCOM is involved. If XPCOM had support for a more
formal exception mechanism, then maybe it could transfer more than
just the rv. Then that way we could transport more information about
the error, and then when the error becomes "unhandled" report that.

[tg...@mozilla.com]

This is still a bad idea. I think we should ban the use of return in
macros altogether except for methods that are wholly constructed by macros.

>
> Mike

[tg...@mozilla.com]

Joshua Cranmer wrote:
> On 09/30/2009 11:31 AM, Benjamin Smedberg wrote:
>> Thoughts? I know a couple people really like the NS_ENSURE_* macros,
>> but I
>> think that overall they are very mozilla-specific and aren't good for
>> making
>> our code more standard c++.
>
> It sounds like I'm going to be in the minority here, but I kind of like
> them.
>
> My rationale:
> * Style guides (last I check) recommend checking the rv after nearly all
> method calls; NS_ENSURE_SUCCESS(rv, rv); is a single line, unlike
> if (NS_FAILED(rv))
> {
> NS_WARNING("rv failed!");
> return rv;
> }

what about combining NS_FAILED with an error message

if (NS_FAILED_ASSERT(rv, "This shouldn't happen")) {
return rv
}

This isn't hard with some , abuse.

Taras