712873 - Use the mfbt runtime assertion macros rather than JS aliases to them

Status: RESOLVED DUPLICATE of bug 1074911

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Now that SpiderMonkey's JS_ASSERT macro straightforwardly expands to a central MOZ_ASSERT for all Mozilla code, we should just use MOZ_ASSERT directly, for greater clarity and fewer layers of abstraction to work through to understand it.  Ditto for JS_ASSERT_IF, mutatis mutandis.  (MOZ_ALWAYS_TRUE and MOZ_ALWAYS_FALSE could use the same as well.)

It's not entirely clear to me whether JS_ASSERT, or JS_ALWAYS_*, were considered fully-deliberate public APIs that we wanted people to use.  I think we might have sort of claimed it by dint of their being in a vaguely public jsutil.h, but I don't see them mentioned in MDN except in an "internals" page.  (Although MDN doesn't even document JS_Assert, so...yeah.)  For now, at least, I'm leaving aliases in place.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Convert JS_ASSERT{,_IF}

I made the patch here with this command at top level of a Mozilla tree (my objdir is ./dbg):

find . \( -path ./dbg -prune -o -path ./.hg -prune \) -o \
       -exec perl -p -i -e 's/JS_ASSERT/MOZ_ASSERT/g' {} \;

Then I undid its changes to the JS_ASSERT{,_IF} definitions, fixed up alignment in the few instances in macros where the \ at the end of the line was now mis-aligned, and reverted changes which were to instances of JS_ASSERT_STRING_IS_FLAT.  Easy and mechanical, mostly unlike the integer types patch.

In theory it'd be clearer for all these users to #include "mozilla/Assertions.h" rather than bootleg it from some random JS header.  It might be nice to have that, but I'd rather not get too picky about doing this to everything (particularly since I'm not sure how you'd best automate that, and by-hand seems a pain/not-good use of time), as it's to a large extent a stylistic change.

239 files changed, 4949 insertions(+), 4948 deletions(-)

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Convert JS_ALWAYS_{TRUE,FALSE}

Same deal for patch generation here, except undoing the changes to the JS_ALWAYS_* definitions was the only extra bit needed.

22 files changed, 49 insertions(+), 49 deletions(-)

Comment 3

[Nicholas Nethercote [inactive]]

I've liked a lot of the clean-ups you've been doing lately, but this one feels unnecessary.  It'll create a lot of version control churn for very little benefit, IMO.

A halfway house I could support would be to make this change outside the JS engine, where using JS_ASSERT is admittedly a bit odd.

Comment 4

[Jeff Walden [:Waldo]]

The benefit is people reading Mozilla code only have to know about one thing, and one implementation of it.  The win here is definitely not "immediate", but longer-term.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 583727 [details] [diff] [review]
Convert JS_ASSERT{,_IF}

No, thank you.

Comment 6

[Luke Wagner [:luke]]

(Agree with comment 3)
I like the JS_ prefix when inside JS engine.  Feel free to move the #define into the "private" (when we un-INSTALLED_HEADER it) jsutil.h and, as njn suggested, kill JS_ASSERT use outside js/src.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: part-1-replace-JS_ASSERT-outside-js-directory.patch

Part 1: Replace JS_ASSERT with MOZ_ASSERT outside js/ directory.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: part-2-replace-JS_ASSERT-outside-js-directory.patch

Part 2: Replace JS_ASSERT with MOZ_ASSERT in js/ directories outside js/src/ directory.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: 712873_part-3-replace-JS_ALWAYS_TRUE-outside-js-src-directory.patch

Part 3: Replace JS_ALWAYS_TRUE with MOZ_ALWAYS_TRUE everywhere outside js/src/ directory.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8377419 [details] [diff] [review]
712873_part-3-replace-JS_ALWAYS_TRUE-outside-js-src-directory.patch

Review of attachment 8377419 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Utility.h
@@ -41,5 @@
>  #define JS_FREE_PATTERN 0xDA
>  
>  #define JS_ASSERT(expr)           MOZ_ASSERT(expr)
>  #define JS_ASSERT_IF(cond, expr)  MOZ_ASSERT_IF(cond, expr)
> -#define JS_ALWAYS_TRUE(expr)      MOZ_ALWAYS_TRUE(expr)

I wouldn't thought you could also move JS_ASSERT(,_IF) to jsutil.h after the first.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Luke Wagner [:luke] from comment #10)
> Comment on attachment 8377419 [details] [diff] [review]
> 712873_part-3-replace-JS_ALWAYS_TRUE-outside-js-src-directory.patch
> 
> Review of attachment 8377419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/Utility.h
> @@ -41,5 @@
> >  #define JS_FREE_PATTERN 0xDA
> >  
> >  #define JS_ASSERT(expr)           MOZ_ASSERT(expr)
> >  #define JS_ASSERT_IF(cond, expr)  MOZ_ASSERT_IF(cond, expr)
> > -#define JS_ALWAYS_TRUE(expr)      MOZ_ALWAYS_TRUE(expr)
> 
> I wouldn't thought you could also move JS_ASSERT(,_IF) to jsutil.h after the
> first.

Not yet. Some js header files (like jsapi.h) are #include by xpcom, which does not have access to jsutil.h.

Comment 12

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39adab158c17
https://hg.mozilla.org/integration/mozilla-inbound/rev/16add78c43e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/37193db7e15b

JS_ASSERT and JS_ASSERT_IF are no longer called from outside the js/ directory, but they are still needed by js/src/*.h headers (like jsapi.h) that are indirectly #included outside the js/ directory.

Comment 13

[Daniel Holbert [:dholbert]]

The changes to JS_DIAGNOSTICS_ASSERT (in part 2) seem to have busted Mac and B2G builds, so far, with errors like:
{
In file included from /builds/slave/m-in-osx64-0000000000000000000/build/obj-firefox/i386/js/src/Unified_cpp_js_src1.cpp:67:
/builds/slave/m-in-osx64-0000000000000000000/build/js/src/gc/Marking.cpp:962:5: error: expected ')'
    JS_DIAGNOSTICS_ASSERT(GetGCThingTraceKind(rope) == JSTRACE_STRING);
    ^
/builds/slave/m-in-osx64-0000000000000000000/build/js/src/jsutil.h:28:69: note: expanded from macro 'JS_DIAGNOSTICS_ASSERT'
# define JS_DIAGNOSTICS_ASSERT(expr) do { if (MOZ_UNLIKELY(!(expr)) MOZ_CRASH(); } while(0)
                                                                    ^
../../dist/include/mozilla/Assertions.h:235:26: note: expanded from macro 'MOZ_CRASH'
#  define MOZ_CRASH(...) MOZ_REALLY_CRASH()
                         ^
../../dist/include/mozilla/Assertions.h:200:8: note: expanded from macro 'MOZ_REALLY_CRASH'
       do { \
       ^
/builds/slave/m-in-osx64-0000000000000000000/build/js/src/gc/Marking.cpp:962:5: note: to match this '('
/builds/slave/m-in-osx64-0000000000000000000/build/js/src/jsutil.h:28:46: note: expanded from macro 'JS_DIAGNOSTICS_ASSERT'
# define JS_DIAGNOSTICS_ASSERT(expr) do { if (MOZ_UNLIKELY(!(expr)) MOZ_CRASH(); } while(0)
                                             ^
}

Sample logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=34895270&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=34895264&tree=Mozilla-Inbound

Comment 14

[Daniel Holbert [:dholbert]]

er sorry, "in part 3" (not part 2)

Comment 15

[Daniel Holbert [:dholbert]]

Backed out for aforementioned bustage (also backed out bug 712939, from the same push, since it also was JS assert related and I wasn't sure if it depended on this bug's patches):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a6425a31c4c4

Comment 16

[Chris Peterson [:cpeterson]]

relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c952f7937ec1
https://hg.mozilla.org/integration/mozilla-inbound/rev/28fa156efc31
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0259d0c1882

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c952f7937ec1
https://hg.mozilla.org/mozilla-central/rev/28fa156efc31
https://hg.mozilla.org/mozilla-central/rev/d0259d0c1882