Gecko NS_ENSURE_* macros
Nicholas Nethercote
This is a follow-up to the "Gecko assertions, errors, warnings, et al" thread
(http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/556cb7
debb174cca#).
Most of the discussion in that thread was about the NS_ENSURE_* macros. I'm
going to make a detailed, specific proposal in an attempt to get some
concrete actions out of the discussion. (It's actually much the same as
what I proposed in the earlier thread, but I'm suggesting it by itself here
to keep things focused.)
The idea is to simply rename NS_ENSURE_*. Specifically:
- NS_ENSURE_TRUE(c, ret) -> NS_WARN_AND_RETURN_IF_FALSE(c, ret)
- NS_ENSURE_FALSE(c, ret) -> NS_WARN_AND_RETURN_IF_TRUE(c, ret)
- NS_ENSURE_SUCCESS(res, ret) -> NS_WARN_AND_RETURN_IF_NOT_OK(res, ret)
I'm happy to bikeshed the exact names but I think it's crucial that "WARN"
and "RETURN" are both there in some form. (Even "NS_WnR_IF_*", short for
"Warn 'n' Return" would be better than the current, IMO.)
Pros of this approach:
- Simplicity. In particular, the change can be made without rewriting tools
that are "almost" ready or would "take probably only a weekend to write";
Perl/sed/whatever will suffice.
- The two surprising aspects of these macros -- that they (a) warn, and (b)
return early -- are explicit. NS_WARN_AND_RETURN_IF_NOT_OK is a bit
non-obvious, due to the two nsresult arguments, but if you compare it with
NS_WARN_AND_RETURN_IF_FALSE you can probably guess that the first argument
is tested and the 2nd argument is returned.
- Preserves the fact that this is a common Mozilla idiom (replacing these
macros with explicit control flow would lose the idiom-ness).
- No changes to how people use debuggers to examine warnings.
Cons of this approach:
- It's more horizontally verbose; however, it's not more vertically verbose,
and I think vertical verbosity is worse than horizontal verbosity. (It
could result in more line-wrapping, true, but a Perl script wouldn't
insert those line-wraps ;)
(The _WnR_ alternatives would be shorter than the current macros, however :)
- Lots of tree-churn (~18,000 changes); but that's inevitable with any change.
Actually, there's a complication -- there are more NS_ENSURE_* macros than
the three I've given. What to do with the others is less clear to me.
Here's some ideas.
// These two appear to be used interchangeably, in seemingly
// type-incorrect ways. Eg. I found pointer arguments passed to both
// very easily. The only difference is the return value in the failure
// case, which is NS_ERROR_INVALID_ARG vs. NS_ERROR_INVALID_POINTER.
- NS_ENSURE_ARG(arg) -> NS_WARN_AND_RETURN_IF_ARG_FALSE(arg)
- NS_ENSURE_ARG_POINTER(arg) -> NS_WARN_AND_RETURN_IF_ARG_NULL(arg)
// These three are only used 59, 9 and 5 times respectively, could be
// removed.
- NS_ENSURE_ARG_MIN(arg) -> NS_WARN_AND_RETURN_IF_ARG_LESS_THAN(arg, min)
- NS_ENSURE_ARG_MAX(arg) -> NS_WARN_AND_RETURN_IF_ARG_GREATER_THAN(arg, max
)
- NS_ENSURE_ARG_RANGE(arg) -> NS_WARN_AND_RETURN_IF_ARG_NOT_IN_RANGE(arg, min
, max)
- NS_ENSURE_STATE(state) -> NS_WARN_AND_RETURN_IF_STATE_FALSE(arg)
[or should that be "_IF_STATE_NULL"?]
// These two are only used 20 and 2 times respectively, could be removed
- NS_ENSURE_NO_AGGREGATION -> remove
- NS_ENSURE_PROPER_AGGREGATION-> remove
Comments welcome.
Nick
Joshua Cranmer
> - The two surprising aspects of these macros -- that they (a) warn, and (b)
> return early -- are explicit. NS_WARN_AND_RETURN_IF_NOT_OK is a bit
> non-obvious, due to the two nsresult arguments, but if you compare it with
> NS_WARN_AND_RETURN_IF_FALSE you can probably guess that the first argument
> is tested and the 2nd argument is returned.
explicitly lays out every action of the macro, we should instead try to
create a name which emphasizes the macros /role/. For example, a better
name might be NS_THROW_EXCEPTION (people introduced to the C++ code in
Mozilla from the JS would better understand that the role of the return
values is the same as vanilla JS exceptions), although people might
expect it to actively throw an exception.
> - It's more horizontally verbose; however, it's not more vertically verbose,
> and I think vertical verbosity is worse than horizontal verbosity. (It
> could result in more line-wrapping, true, but a Perl script wouldn't
> insert those line-wraps ;)
> (The _WnR_ alternatives would be shorter than the current macros, however :)
virtually any depth; since Mozilla code by and large keeps to ~80 width
source files, anything other 30 characters for the macro name is just
asking for trouble, and I would argue the name should be shorter than 20
if at all possible. Since these are probably one of the most heavily
used Mozilla idioms, having to wrap it onto multiple lines gets really
annoying really quickly. I also want to be able to type them reliably
without having to resort to copy-paste or [editor] macros.
NS_ENSURE_SUCCESS is easy to fit into that build, but
NS_WARN_AND_RETURN_IF_FALSE is not (it also, incidentally, uses a wider
variety of characters: the former just 6, the latter 14).
> - NS_ENSURE_ARG_MAX(arg) -> NS_WARN_AND_RETURN_IF_ARG_GREATER_THAN(arg, max
> )
> - NS_ENSURE_ARG_RANGE(arg) -> NS_WARN_AND_RETURN_IF_ARG_NOT_IN_RANGE(arg, min
> , max)
Nicholas Nethercote
>
> I would argue that instead of trying to come up with a long name that
> explicitly lays out every action of the macro, we should instead try to
> create a name which emphasizes the macros /role/. For example, a better name
> might be NS_THROW_EXCEPTION (people introduced to the C++ code in Mozilla
> from the JS would better understand that the role of the return values is
> the same as vanilla JS exceptions), although people might expect it to
> actively throw an exception.
Which is one of the problems I think the ENSURE variants have (I
assumed they were some kind of assertion) and which I'm trying to
avoid.
> I think an amount of terseness is necessary
Maybe the _WnR_ variants would be ok, then?
- They're terser than the current macros, so that's good.
- The "WnR" mnemonic is useful for people who are familiar with them;
several people mentioned that they keep forgetting what the _ENSURE_
macros do exactly.
- The "WnR" is weird enough that newbies are likely to look up what
they mean, which doesn't appear to be the case with _ENSURE_ (speaking
for myself, I just thought "oh, I guess that's some kind of
assertion).
Nick
Jonas Sicking
<n.neth...@gmail.com> wrote:
> Hi,
>
> This is a follow-up to the "Gecko assertions, errors, warnings, et al" thread
> (http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/556cb7
> debb174cca#).
>
> Most of the discussion in that thread was about the NS_ENSURE_* macros. I'm
> going to make a detailed, specific proposal in an attempt to get some
> concrete actions out of the discussion. (It's actually much the same as
> what I proposed in the earlier thread, but I'm suggesting it by itself here
> to keep things focused.)
>
> The idea is to simply rename NS_ENSURE_*. Specifically:
>
> - NS_ENSURE_TRUE(c, ret) -> NS_WARN_AND_RETURN_IF_FALSE(c, ret)
> - NS_ENSURE_FALSE(c, ret) -> NS_WARN_AND_RETURN_IF_TRUE(c, ret)
> - NS_ENSURE_SUCCESS(res, ret) -> NS_WARN_AND_RETURN_IF_NOT_OK(res, ret)
>
> I'm happy to bikeshed the exact names but I think it's crucial that "WARN"
> and "RETURN" are both there in some form. (Even "NS_WnR_IF_*", short for
> "Warn 'n' Return" would be better than the current, IMO.)
>
> Pros of this approach:
>
> - Simplicity. In particular, the change can be made without rewriting tools
> that are "almost" ready or would "take probably only a weekend to write";
> Perl/sed/whatever will suffice.
>
> return early -- are explicit. NS_WARN_AND_RETURN_IF_NOT_OK is a bit
> non-obvious, due to the two nsresult arguments, but if you compare it with
> NS_WARN_AND_RETURN_IF_FALSE you can probably guess that the first argument
> is tested and the 2nd argument is returned.
>
> macros with explicit control flow would lose the idiom-ness).
>
> - No changes to how people use debuggers to examine warnings.
>
> Cons of this approach:
>
> and I think vertical verbosity is worse than horizontal verbosity. (It
> could result in more line-wrapping, true, but a Perl script wouldn't
> insert those line-wraps ;)
> (The _WnR_ alternatives would be shorter than the current macros, however :)
>
>
>
> Actually, there's a complication -- there are more NS_ENSURE_* macros than
> the three I've given. What to do with the others is less clear to me.
> Here's some ideas.
>
> // These two appear to be used interchangeably, in seemingly
> // type-incorrect ways. Eg. I found pointer arguments passed to both
> // very easily. The only difference is the return value in the failure
> // case, which is NS_ERROR_INVALID_ARG vs. NS_ERROR_INVALID_POINTER.
> - NS_ENSURE_ARG(arg) -> NS_WARN_AND_RETURN_IF_ARG_FALSE(arg)
> - NS_ENSURE_ARG_POINTER(arg) -> NS_WARN_AND_RETURN_IF_ARG_NULL(arg)
>
> // These three are only used 59, 9 and 5 times respectively, could be
> // removed.
> - NS_ENSURE_ARG_MIN(arg) -> NS_WARN_AND_RETURN_IF_ARG_LESS_THAN(arg, min)
> )
> - NS_ENSURE_ARG_RANGE(arg) -> NS_WARN_AND_RETURN_IF_ARG_NOT_IN_RANGE(arg, min
> , max)
>
> [or should that be "_IF_STATE_NULL"?]
>
> // These two are only used 20 and 2 times respectively, could be removed
> - NS_ENSURE_NO_AGGREGATION -> remove
> - NS_ENSURE_PROPER_AGGREGATION-> remove
>
>
> Comments welcome.
Several people expressed strong dislike for the use of macros which
hide code flow. To the point that they don't think it's worth changing
from one bad solution, to another equally bad but with a different
name, or even to the point that they actively don't use the macro at
all.
Your above suggestion doesn't seem to take into account that feedback.
As far as I could tell, no one expressed strong negative opinion of a
solution like:
if (NS_WARN_FAILURE(rv))
return rv;
or
if (warn_failure(rv))
return rv;
Is there a reason you didn't go with that proposal?
/ Jonas
Nicholas Nethercote
>
> Several people expressed strong dislike for the use of macros which
> hide code flow.
I think there was a mixture of opinions, and that some people thought
it was ok if the name made the early return clear. I'm in that camp.
> As far as I could tell, no one expressed strong negative opinion of a
> solution like:
>
> if (NS_WARN_FAILURE(rv))
> return rv;
>
> or
>
> if (warn_failure(rv))
> return rv;
>
> Is there a reason you didn't go with that proposal?
Someone did complain about this, or something very similar: you!
Quoting from the other thread:
On Sat, Jun 11, 2011 at 5:59 AM, Jonas Sicking <jo...@sicking.cc> wrote:
> 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.
That was one of the arguments that I paid most attention to.
Have you changed your mind?
Nick
Jonas Sicking
To quote another email in the same thread from me:
"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;
..."
Given the number of people that opposed changing the name of the
macros (which I'm not a fan of either FWIW as it doesn't seem to solve
any real problems), I don't think we can ignore those arguments.
/ Jonas
Nicholas Nethercote
>
> As far as I could tell, no one expressed strong negative opinion of a
> solution like:
>
> if (NS_WARN_FAILURE(rv))
> return rv;
>
> or
>
> if (warn_failure(rv))
> return rv;
>
> Is there a reason you didn't go with that proposal?
I think that would also be an improvement over the status quo. I
didn't personally push for it because I don't know anything about
rewriting tools (which would be required for this change, AFAICT) and
I didn't want to propose a change that I couldn't myself implement,
because I consider that a recipe for inaction.
If you want to push for this proposal, and you can get agreement, and
you know how to implement it, please do! I suggest you start with a
detailed description of how every relevant macro would be changed.
But also don't forget about the people like Ehsan and Smaug who said
they like the existing macros and didn't want them changed.
Nick
Mounir Lamouri
> As far as I could tell, no one expressed strong negative opinion of a
> solution like:
>
> if (NS_WARN_FAILURE(rv))
> return rv;
>
> or
>
> if (warn_failure(rv))
> return rv;
In case of this proposition makes it (which would be great), I would
like to point that the name "NS_WARN_FAILURE" is probably too close of
"NS_ERROR_FAILURE which is one possible value of |rv|. To prevent any
confusion, we should probably try to use something closer to "NS_FAILED"
(basically, this macro would be a NS_FAILED() check and a NS_WARNING()
call).
I think "NS_WARN_FAILED" would be better but maybe
NS_FAILED_WITH_WARNING or NS_CHECK_FAILURE_WITH_WARNING?
We can even have a method like the NS_FAILED() macro that would take the
verbosity level in parameter and do nothing, warn, show an error or
assert... But I'm thinking out loud here.
--
Mounir
Ehsan Akhgari
> On Tue, Jun 14, 2011 at 5:46 PM, Jonas Sicking<jo...@sicking.cc> wrote:
>>
>> As far as I could tell, no one expressed strong negative opinion of a
>> solution like:
>>
>> if (NS_WARN_FAILURE(rv))
>> return rv;
>>
>> or
>>
>> if (warn_failure(rv))
>> return rv;
>>
>
> I think that would also be an improvement over the status quo. I
> didn't personally push for it because I don't know anything about
> rewriting tools (which would be required for this change, AFAICT) and
> I didn't want to propose a change that I couldn't myself implement,
> because I consider that a recipe for inaction.
I don't think that the tiny improvement that your proposal provides
(marginally better names for _some_ of the macros) outweighs the churn
in the tree, which could have otherwise been avoided. More
specifically, (1) a name like NS_WARN_AND_RETURN_IF_NOT_OK is not really
clearer, (2) newcomers would still need to look up their definitions to
figure out the meaning of the two arguments, and still have to remember
their meaning/ordering, and (3) they're much harder to type and break
people's habits and existing out-of-the-tree patches (the latter is a
problem with any such change which would be worthwhile only if the
change improves things noticeably).
I don't think that we should necessarily opt for solutions which can be
implemented in sed. Not changing anything is always an option, changing
things without making them better should not be what we aim for. :-)
Ehsan
Jonas Sicking
So if we can find someone to do the work, is anyone opposed to making
the switch to
if (warn_failed(rv))
return rv;
if (warn_false(cond))
return NS_ERROR_FAILED;
if (warn_true(cond))
return NS_ERROR_FAILED;
One thing to note though is that it likely wouldn't be possible to
follow the bracing style of the file as that requires human analysis
of every file changed. So the above bracing is likely what would go
everywhere.
We do have tools to get indentation correct though, so that shouldn't
be a problem.
(as a side note, I suspect that sed + reindentation tool is likely
enough to make this change).
/ Jonas
Joshua Cranmer
> So if we can find someone to do the work, is anyone opposed to making
> the switch to
>
> if (warn_failed(rv))
> return rv;
>
> if (warn_false(cond))
> return NS_ERROR_FAILED;
>
> if (warn_true(cond))
> return NS_ERROR_FAILED;
Would it be too much to ask if, for these cases only, we could relax the
style rules to permit:
if (warn_failed(rv)) return rv;
?
With that caveat, I would not be opposed. Without it... I'm not sure.
Kyle Huey
> So if we can find someone to do the work, is anyone opposed to making
> the switch to
>
> if (warn_failed(rv))
> return rv;
>
> if (warn_false(cond))
> return NS_ERROR_FAILED;
>
> if (warn_true(cond))
> return NS_ERROR_FAILED;
>
I'll be the voice of dissent here. I really don't have a problem with
macros, even macros that hide control flow, if they hide stuff that's boring
and repetitive.
If people want to change the names of the macros I'm not going to lose much
sleep over it. (I'd suggest NS_PROPAGATE_FAILURE (or even just
PROPAGATE_FAILURE!) or THROW_IF_FAILED) I am, however, opposed to writing
out a bunch of boilerplate all the time just so that that boilerplate is
visible.
I don't think we're attacking a real problem here.
- Kyle
Justin Lebar
>
> if (warn_failed(rv))
> return rv;
> if (warn_false(cond))
> return NS_ERROR_FAILED;
> if (warn_true(cond))
> return NS_ERROR_FAILED;
I don't think "warn_failed" is as user-friendly as "warn_if_failed". But neither one is entirely clear -- what's the return value of warn_if_false(true)? Is it true because it warned, or false because the condition !cond was false?
I really don't think this is an improvement over
NS_ENSURE_STATE(!cond);
But if we're going to do it, can we consider putting these all on one line? Our code density sucks as it is.
Last, I want to question the wisdom that we can usefully differentiate between "expected" and "unexpected" failures. For instance, history.pushState bails with NS_ERROR_ILLEGAL_VALUE when the object you're pushing is too large.
Is this an expected failure? Well, yes, we explicitly check for this condition. But it's also a rare condition, and if it gets hit in a debug build, I'd like to know about it, because it probably indicates that something is broken. I could output a warning to PR logging, but that feels like overkill, especially since I don't expect to turn on PR logging just to look for this failure.
I buy that there are NS_ENSURE_* calls which are spammy and should be changed to something else. But I don't see why we should get rid of the macro everywhere because 0.1% of callsites are bad.
Jeff Muizelaar
On 2011-06-14, at 12:59 PM, Jonas Sicking wrote:
> So if we can find someone to do the work, is anyone opposed to making
> the switch to
>
> if (warn_failed(rv))
> return rv;
>
> if (warn_false(cond))
> return NS_ERROR_FAILED;
>
> if (warn_true(cond))
> return NS_ERROR_FAILED;
I'm sure there will always be people opposed to any particular change, so I don't think we should necessarily look for complete consensus. I'm not sure what criteria we need to actually make a decision however, I'd like to register a strong vote in favour of this change.
-Jeff
Jonas Sicking
There are two problems with that.
First of it means that it's not possible to put a breakpoint on the
return statement in a debugger (this is something that bothers me
every now and then with our current macros).
Second, it doesn't follow the indentation practice of almost any files
in our tree.
/ Jonas
Justin Lebar
> First of it means that it's not possible to put a breakpoint on the
> return statement in a debugger (this is something that bothers me
> every now and then with our current macros).
You can set a conditional breakpoint on the line to accomplish the same thing.
> Second, it doesn't follow the indentation practice of almost any files
> in our tree.
Correct. The question is: Is this inconsistency worth increased code density? I think it is.
Robert O'Callahan
> On 2011-06-14, at 12:59 PM, Jonas Sicking wrote:
>
> > So if we can find someone to do the work, is anyone opposed to making
> > the switch to
> >
> > if (warn_failed(rv))
> > return rv;
> >
> > if (warn_false(cond))
> > return NS_ERROR_FAILED;
> >
> > if (warn_true(cond))
> > return NS_ERROR_FAILED;
>
> I'm sure there will always be people opposed to any particular change, so I
> don't think we should necessarily look for complete consensus. I'm not sure
> what criteria we need to actually make a decision however, I'd like to
> register a strong vote in favour of this change.
>
Me too!
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]
Steve Fink
Detailed description:
NS_ENSURE_TRUE(cond,rv) ->
if (NS_WARN_IF_NOT(cond))
return rv
NS_ENSURE_FALSE(cond,rv) ->
if (NS_WARN_IF(cond))
return rv
NS_ENSURE_SUCCESS(rv1, rv2) ->
if (NS_WARN_IF_FAIL(rv1))
return rv2
NS_ENSURE_{ARG,POINTER,STATE}(cond) ->
if (NS_WARN_IF_NOT(cond))
return NS_ERROR_*
Result:
1161 files changed, 35959 insertions(+), 18262 deletions(-)
plus about 50 unhandled cases that need to be looked at manually. Of
those, most are macro definitions that include NS_ENSURE_*. I could
handle those with a bit more work. Several are the locally-defined
NS_ENSURE_SUBMIT_SUCCESS in nsHTMLFormElement.cpp that does some cleanup
but notably does not warn. There were only two occurrences of an 'else'
statement that would be incorrectly captured by this rewrite. (According
to my script, anyway.)
I'm not thrilled with the names, but they still seem preferable to what
they're replacing. Maybe lowercase is better. I didn't think too hard
about it; up until the final run I was doing
NS_ENSURE_{TRUE,FALSE,SUCCESS} -> warn_if_{OOGA,BOOGA,WOOGA}, which I
hereby submit for consideration. NS_NOISY_CHECK, warn_check, ...?
People who like the current macros:
Ehsan, smaug, whoever: take a look at
http://people.mozilla.org/~sfink/uploads/rewrite.diff.gz and see how it
strikes you.
jlebar: Yes, the returns are on a separate line. It would simplify the
script to put them on the same line, so it'd be easy to change, but I
still like having them separate better.
khuey: Look, a squirrel!
A snapshot of the rewrite script is at
http://people.mozilla.org/~sfink/uploads/rename-ensure for the curious.
Steve Fink
Detailed description:
Nicholas Nethercote
>
> Detailed description:
>
> NS_ENSURE_TRUE(cond,rv) ->
> if (NS_WARN_IF_NOT(cond))
> return rv
>
> NS_ENSURE_FALSE(cond,rv) ->
> if (NS_WARN_IF(cond))
> return rv
>
> NS_ENSURE_SUCCESS(rv1, rv2) ->
> if (NS_WARN_IF_FAIL(rv1))
> return rv2
>
> NS_ENSURE_{ARG,POINTER,STATE}(cond) ->
> if (NS_WARN_IF_NOT(cond))
> return NS_ERROR_*
Don't forget these ones:
- NS_ENSURE_ARG_MIN
- NS_ENSURE_ARG_MAX
- NS_ENSURE_ARG_RANGE
- NS_ENSURE_NO_AGGREGATION
- NS_ENSURE_PROPER_AGGREGATION
Nick
Boris Zbarsky
> [On the subject of: Should there be a newline after the if statement?]
>
>> First of it means that it's not possible to put a breakpoint on the
>> return statement in a debugger (this is something that bothers me
>> every now and then with our current macros).
>
> You can set a conditional breakpoint on the line to accomplish the same thing.
"Maybe".
Conditional breakpoints, at least in gdb, are very slow. If your code
is hot (think somewhere in selector matching), then it's at least two
orders of magnitude faster to change the code so you can set a
breakpoint on a line that's only hit if your condition is true,
recompile the whole browser, run, set the unconditional breakpoint, and
reproduce the bug then do the same using a conditional breakpoint.
I say "at least" because last time I was dealing with this I killed the
conditional breakpoint version after about two hours without it ever
getting far enough in the "start up and load this one web page" process
to actually reproduce the bug... who knows how long it would have taken
to get there.
-Boris
Shawn Wilsher
> follow the bracing style of the file as that requires human analysis
> of every file changed. So the above bracing is likely what would go
> everywhere.
that all ifs should be braced, so not only would we not be following
local style, but we wouldn't even be following the style in the style
guide. (yup, I just brought up the style guide)
Lastly, this change effectively increases the length of any code that
uses the storage API by 50% to get or set data in a database, which is
going to be unpleasant for the, admittedly, few people who hack on code
that uses it these days.
Cheers,
Shawn
Jason Duell
> NS_ENSURE_TRUE(cond,rv) ->
> if (NS_WARN_IF_NOT(cond))
> return rv
s/NS_WARN_IF_NOT/NS_WARN_UNLESS/ ?
Jason
Jeff Muizelaar
On 2011-06-14, at 8:48 PM, Steve Fink wrote:
> Ehsan, smaug, whoever: take a look at http://people.mozilla.org/~sfink/uploads/rewrite.diff.gz and see how it strikes you.
the gfx/ parts of this look good to me. I did notice an extra space after empty return statements. e.g. "return ;" vs. "return;"
-Jeff
Jonas Sicking
Can we please avoid the NS_ prefix, and the screaming uppercases
(while technically these have to be macros in debug builds so that we
can grab the line number and file name, I don't think that knowing the
difference is making anyone happier).
Also, if(warn_if_not(...)) reads very strange to me, but I care less about that.
/ Jonas
Steve Fink
I fixed the 'return ;' thing, and made a version with warn_if,
warn_unless, and warn_if_fail. You can see how that looks at
http://people.mozilla.org/~sfink/uploads/rewrite2.diff.gz
if (check_and_warn(...)) return bleck;
maybe?
(I'd really like to do something like |warn_unless(cond) && return
bleck| but C++ doesn't like that.)
Or maybe warn_if_false in place of warn_unless/warn_if_not?
warn_on_false? false_and_warn? suckage_when?
Steve Fink
I fixed the 'return ;' thing, and made a version with warn_if,
Robert O'Callahan
alien. WarnIfFalse is more familiar, but I prefer WARN_IF_FALSE myself.
Benjamin Smedberg
> We don't use lowercase plus underscores in Gecko. So warn_if_false is quite
> alien. WarnIfFalse is more familiar, but I prefer WARN_IF_FALSE myself.
function, so caps plus underscores are still a bit foreign.
--BDS
Robert Kaiser
> - It's more horizontally verbose; however, it's not more vertically verbose,
> and I think vertical verbosity is worse than horizontal verbosity. (It
> could result in more line-wrapping, true, but a Perl script wouldn't
> insert those line-wraps ;)
That sounds like that script would break code style due to line length
overruns, which usually can be enough for r- on its own... ;-)
I tend to wonder if a macro that expands to just two rather simple lines
in all cases is really worth the effort of hiding the actual calls
behind it.
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! :)
Ehsan Akhgari
Wouldn't it? Using an inline function means that we lose the file name
and line number information. I don't think we should do that, at all.
With this being a macro, the unprefixed uppercase versions look good to me.
Also, I don't think that we want to land a giant patch for the whole
Gecko. Let's get bugs filed on it for each module, and let the module
owners deal with the patches in their own code. But first we have to
settle on the naming issue. :-)
Ehsan
Ehsan Akhgari
> On 6/14/2011 9:59 AM, Jonas Sicking wrote:
>> One thing to note though is that it likely wouldn't be possible to
>> follow the bracing style of the file as that requires human analysis
>> of every file changed. So the above bracing is likely what would go
>> everywhere.
> I'd just like to point out that the coding style guide actually says
> that all ifs should be braced, so not only would we not be following
> local style, but we wouldn't even be following the style in the style
> guide. (yup, I just brought up the style guide)
Guess who can edit the style guidelines to make an exception for error
propagation checks? :-)
Ehsan
Daniel Holbert
As long as we're converting to "if" terminology (ENSURE_TRUE -> WARN_IF),
I think we should drop the ENSURE_FALSE variant, and replace it with
"WARN_IF(!condition)".
The "WARN_UNLESS"/"WARN_IF_NOT" variant would be extraneous, and I
actually worry that it'd cause more confusion than it's worth.
In particular -- if we *did* add an UNLESS variant, then
if (WARN_UNLESS(condition))
return rv;
is hard to interpret correctly. i.e. what value gets propagated up to the
actual "if" check -- "condition", or "!condition"? (To preserve the
NS_ENSURE_FALSE behavior, I think it needs to be "!condition", but that's
not entirely intuitive from first glance.)
Compare instead to:
if (WARN_IF(!condition))
return rv;
Much clearer, no? :)
~Daniel
Steve Fink
> On 06/14/2011 08:05 PM, Jason Duell wrote:
>> On Jun 14, 5:48 pm, Steve Fink<sf...@mozilla.com> wrote:
>>
>>> NS_ENSURE_TRUE(cond,rv) ->
>>> if (NS_WARN_IF_NOT(cond))
>>> return rv
>>
>> s/NS_WARN_IF_NOT/NS_WARN_UNLESS/ ?
>
> As long as we're converting to "if" terminology (ENSURE_TRUE ->
> WARN_IF), I think we should drop the ENSURE_FALSE variant, and replace
> it with "WARN_IF(!condition)".
Except it's the other way around. NS_ENSURE_TRUE(WishfulThinking) maps
to WARN_IF_NOT/WARN_UNLESS(WishfulThinking). NS_ENSURE_FALSE(badness)
maps to WARN_IF(badness).
> The "WARN_UNLESS"/"WARN_IF_NOT" variant would be extraneous, and I
> actually worry that it'd cause more confusion than it's worth.
>
> In particular -- if we *did* add an UNLESS variant, then
> if (WARN_UNLESS(condition))
> return rv;
> is hard to interpret correctly. i.e. what value gets propagated up to
> the actual "if" check -- "condition", or "!condition"? (To preserve the
> NS_ENSURE_FALSE behavior, I think it needs to be "!condition", but
> that's not entirely intuitive from first glance.)
>
> Compare instead to:
> if (WARN_IF(!condition))
> return rv;
> Much clearer, no? :)
Sure, but it means inverting nearly every condition in the tree (since
NS_ENSURE_TRUE is far more common than NS_ENSURE_FALSE.)
Right now, having the pair of NS_ENSURE_{TRUE,FALSE} means you can write
your condition whichever way you want, and clearly people prefer to
write what they expect to be true. But that's also an artifact of the
word "ensure". If the macro ends up resembling "WARN_IF", it's naturally
the opposite meaning, so perhaps people would tend to write it the other
way.
Automatically rewriting conditions with dumb regexes may not produce the
most beautiful output, but you can see how my attempt looks at
http://people.mozilla.org/~sfink/uploads/rewrite3.diff.gz
The rewritten conditions actually look ok to me. And I really do dislike
both IF_NOT and UNLESS.
Steve Fink
>> On Jun 14, 5:48 pm, Steve Fink<sf...@mozilla.com> wrote:
>>
>>> NS_ENSURE_TRUE(cond,rv) ->
>>> if (NS_WARN_IF_NOT(cond))
>>> return rv
>>
>> s/NS_WARN_IF_NOT/NS_WARN_UNLESS/ ?
>
> As long as we're converting to "if" terminology (ENSURE_TRUE ->
> WARN_IF), I think we should drop the ENSURE_FALSE variant, and replace
> it with "WARN_IF(!condition)".
Except it's the other way around. NS_ENSURE_TRUE(WishfulThinking) maps
to WARN_IF_NOT/WARN_UNLESS(WishfulThinking). NS_ENSURE_FALSE(badness)
maps to WARN_IF(badness).
> The "WARN_UNLESS"/"WARN_IF_NOT" variant would be extraneous, and I
> actually worry that it'd cause more confusion than it's worth.
>
> In particular -- if we *did* add an UNLESS variant, then
> if (WARN_UNLESS(condition))
> return rv;
> is hard to interpret correctly. i.e. what value gets propagated up to
> the actual "if" check -- "condition", or "!condition"? (To preserve the
> NS_ENSURE_FALSE behavior, I think it needs to be "!condition", but
> that's not entirely intuitive from first glance.)
>
> Compare instead to:
> if (WARN_IF(!condition))
> return rv;
> Much clearer, no? :)
Sure, but it means inverting nearly every condition in the tree (since
Justin Dolske
> Can we please avoid the NS_ prefix
Despite my reluctance to prolong this thread, +1.
Netscape has been dead long enough that it's a bit silly to retain it
for anything new. ;)
Justin
Benjamin Smedberg
> On 11-06-15 9:09 AM, Benjamin Smedberg wrote:
>> On 6/15/2011 6:06 AM, Robert O'Callahan wrote:
>>> We don't use lowercase plus underscores in Gecko. So warn_if_false is
>>> quite
>>> alien. WarnIfFalse is more familiar, but I prefer WARN_IF_FALSE myself.
>> I don't much care, but this wouldn't be a macro, it would be an inline
>> function, so caps plus underscores are still a bit foreign.
>
> Wouldn't it? Using an inline function means that we lose the file
> name and line number information. I don't think we should do that, at
> all.
with just a macro which appears to return a value without
double-evaluating the operand, which is obviously bad.
--BDS
Benjamin Smedberg
fairly easy for macros to conflict with system headers (SUCCESS and
FAILED are MS macros, NS_DEPRECATED is oddly enough a mac system macro).
I think we should keep prefixes for all our macros.
--BDS
Joshua Cranmer
Perhaps you should start using MOZ_* as a macro prefix instead of NS,
since Apple seems to be co-opting that prefix for some reason :-)
Robert Kaiser
> Netscape has been dead long enough that it's a bit silly to retain it
> for anything new. ;)
..and I thought "ns" or "NS" just meant "Namespace" nowadays ;-)
Dao
> Justin Dolske schrieb:
>> Netscape has been dead long enough that it's a bit silly to retain it
>> for anything new. ;)
>
> ..and I thought "ns" or "NS" just meant "Namespace" nowadays ;-)
In that case I recommend we go for NS_NS_WARN_AND_RETURN_IF_FALSE.
Mook
> Also, I don't think that we want to land a giant patch for the whole
> Gecko. Let's get bugs filed on it for each module, and let the module
> owners deal with the patches in their own code. But first we have to
> settle on the naming issue. :-)
That means the old macros will remain for a bit (deprecated) for
compatibility reasons?
The only reason I ask is because this delays the urgent need to suddenly
rewrite any downstream codebases (some of which might not compile with
GCC, no matter how awesome mingw is these days) in order to upgrade
Gecko versions.
--
Mook
Ehsan Akhgari
> On 6/15/2011 7:42 AM, Ehsan Akhgari wrote:
>
>> Also, I don't think that we want to land a giant patch for the whole
>> Gecko. Let's get bugs filed on it for each module, and let the module
>> owners deal with the patches in their own code. But first we have to
>> settle on the naming issue. :-)
>
> That means the old macros will remain for a bit (deprecated) for
> compatibility reasons?
Yes, I think so. I don't think we need to take them away immediately.
For a while we can rely on reviewers and grep to catch new usages
getting into the tree.
Ehsan
Jeff Muizelaar
On 2011-06-15, at 2:17 PM, Steve Fink wrote:
> Automatically rewriting conditions with dumb regexes may not produce the most beautiful output, but you can see how my attempt looks at http://people.mozilla.org/~sfink/uploads/rewrite3.diff.gz
Can we move forward with this patch?
-Jeff
Taras Glek
Steve said that he can do the gruntwork part of this if someone else
decides on the macro names, etc. I volunteered to be the initial
decider and to help push the review process along.
https://bugzilla.mozilla.org/show_bug.cgi?id=672843
Steve's plan to get back to this in the second part of September.
Taras