Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS

253 views
Skip to first unread message

Benjamin Smedberg

unread,
Nov 22, 2013, 3:18:13 PM11/22/13
to dev-pl...@lists.mozilla.org
With the landing of bug 672843, the NS_ENSURE_* macros are now
considered deprecated. If you are writing code that wants to issue
warnings when methods fail, please either use NS_WARNING directly or use
the new NS_WARN_IF macro.

if (NS_WARN_IF(somethingthatshouldbetrue))
return NS_ERROR_INVALID_ARG;

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

I am working on a script which can be used to automatically convert most
of the existing NS_ENSURE_* macros, and I will also be updating the
coding style guide to point to the recommended form.

--BDS

Daniel Holbert

unread,
Nov 22, 2013, 4:35:55 PM11/22/13
to Benjamin Smedberg, dev-pl...@lists.mozilla.org
On 11/22/2013 12:18 PM, Benjamin Smedberg wrote:
> If you are writing code that wants to issue
> warnings when methods fail, please either use NS_WARNING directly or use
> the new NS_WARN_IF macro.
>
> if (NS_WARN_IF(somethingthatshouldbetrue))
> return NS_ERROR_INVALID_ARG;
>
> if (NS_WARN_IF(NS_FAILED(rv))
> return rv;

I think you mean "somethingthatshouldNOTbetrue" in the first example
there, right?

(Since it looks like we're warning & returning an error code, when that
placeholder is truthy.)

Benjamin Smedberg

unread,
Nov 22, 2013, 4:46:41 PM11/22/13
to Daniel Holbert, dev-pl...@lists.mozilla.org
On 11/22/2013 4:35 PM, Daniel Holbert wrote:
> I think you mean "somethingthatshouldNOTbetrue" in the first example
> there, right?
Indeed.

--BDS

smaug

unread,
Jan 6, 2014, 7:43:41 PM1/6/14
to
Why this deprecation?

Karl Tomlinson

unread,
Jan 6, 2014, 7:58:59 PM1/6/14
to
smaug <sm...@welho.com> writes:

> Why this deprecation?

NS_ENSURE_ macros hid return paths.
Also many people didn't understand that they issued warnings, and
so used the macros for expected return paths.

Was there some useful functionality that is not provided by the
replacements?

smaug

unread,
Jan 6, 2014, 8:04:26 PM1/6/14
to
no, since it is always possible to expand those macros.
However
if (NS_WARN_IF(NS_FAILED(rv)) {
return rv;
}
is super ugly.

Hopefully something like NS_WARN_IF_FAILED(rv) could be added.

Bobby Holley

unread,
Jan 7, 2014, 1:46:31 AM1/7/14
to smaug, dev-pl...@lists.mozilla.org
On Mon, Jan 6, 2014 at 5:04 PM, smaug <sm...@welho.com> wrote:

> no, since it is always possible to expand those macros.
> However
> if (NS_WARN_IF(NS_FAILED(rv)) {
> return rv;
> }
> is super ugly.
>

Note that there in a explicit stylistic exception that NS_WARN_IF
statements do not require braces. So it's:

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


Also, I agree that we should get NS_WARN_IF_FAILED. Then it becomes:

if (NS_WARN_IF_FAILED(rv))
return rv;

which is almost as palatable as NS_ENSURE_SUCCESS(rv, rv);

bholley

Karl Tomlinson

unread,
Jan 7, 2014, 5:37:50 AM1/7/14
to
Bobby Holley writes:

> Note that there in a explicit stylistic exception that NS_WARN_IF
> statements do not require braces. So it's:
>
> if (NS_WARN_IF(NS_FAILED(rv)))
> return rv;

I don't see that on the current version of
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

I like having entirely blank lines (without braces) after jump
statements because I find it helps make them stand out, but I
don't see any reason why there should be an exception only for
particular macros.

smaug

unread,
Jan 7, 2014, 10:14:48 AM1/7/14
to
On 01/07/2014 08:46 AM, Bobby Holley wrote:
> On Mon, Jan 6, 2014 at 5:04 PM, smaug <sm...@welho.com> wrote:
>
>> no, since it is always possible to expand those macros.
>> However
>> if (NS_WARN_IF(NS_FAILED(rv)) {
>> return rv;
>> }
>> is super ugly.
>>
>
> Note that there in a explicit stylistic exception that NS_WARN_IF
> statements do not require braces. So it's:
No exceptions. always {} with if.

smaug

unread,
Jan 7, 2014, 10:26:40 AM1/7/14
to
And looks like whoever updated the coding style made the examples inconsistent, since there has pretty much always
(I think since the time coding style was somewhere in www.mozilla.org/*) been the following "Always brace controlled statements", and
that is still there. No exceptions. Consistency is more important than style.




Andrew McCreight

unread,
Jan 7, 2014, 1:25:47 PM1/7/14
to dev-pl...@lists.mozilla.org
I filed bug 957201 for NS_WARN_IF_FAILED.

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

Bobby Holley

unread,
Jan 7, 2014, 1:34:00 PM1/7/14
to smaug, dev-pl...@lists.mozilla.org
Hm. It's pretty unfortunate that we now need 4 lines per fallible call, as
opposed to 2 with NS_ENSURE_* macros. The stylistic exception made this 3,
which was slightly more palatable.

bholley

Benjamin Smedberg

unread,
Jan 7, 2014, 3:13:31 PM1/7/14
to smaug, dev-pl...@lists.mozilla.org
On 1/6/2014 7:43 PM, smaug wrote:
>
>
> Why this deprecation?

Karl is right, we are removing the macros that hide control flow (as
well as warnings, in this case).

I'm considering the NS_WARN_IF_FAILED(rv) proposal. It's obviously a
less typing then NS_WARN_IF(NS_FAILED(rv)), but I'm not convinced that
it's clear just by reading it what the return type would be, especially
since the signature of NS_WARN_IF returns the same value that is passed in:

bool NS_WARN_IF(bool);

I'm trying to figure out if people would expect the result of
NS_WARN_IF_FAILED to be an nsresult or a bool.

--BDS

Zack Weinberg

unread,
Jan 15, 2014, 10:17:58 PM1/15/14
to
On 2014-01-06 7:58 PM, Karl Tomlinson wrote:
> smaug <sm...@welho.com> writes:
>
>> Why this deprecation?
>
> NS_ENSURE_ macros hid return paths.

That was exactly why they were a Good Thing! Normal control flow was
emphasized.

zw

0 new messages