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

RESOLVED DUPLICATE of bug 1074911

Status

()

defect
--
minor
RESOLVED DUPLICATE of bug 1074911
8 years ago
4 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open])

Attachments

(5 attachments)

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.
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(-)
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #583727 - Flags: review?(luke)
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(-)
Attachment #583728 - Flags: review?(luke)
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.
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 on attachment 583727 [details] [diff] [review]
Convert JS_ASSERT{,_IF}

No, thank you.
Attachment #583727 - Flags: review?(luke) → review-
Attachment #583728 - Flags: review?(luke) → review-
(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.
Part 1: Replace JS_ASSERT with MOZ_ASSERT outside js/ directory.
Attachment #8377415 - Flags: review?(luke)
Part 2: Replace JS_ASSERT with MOZ_ASSERT in js/ directories outside js/src/ directory.
Attachment #8377418 - Flags: review?(luke)
Part 3: Replace JS_ALWAYS_TRUE with MOZ_ALWAYS_TRUE everywhere outside js/src/ directory.
Attachment #8377419 - Flags: review?(luke)
Attachment #8377415 - Flags: review?(luke) → review+
Attachment #8377418 - Flags: review?(luke) → review+
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.
Attachment #8377419 - Flags: review?(luke) → review+
(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.
Attachment #8377418 - Attachment description: part-2-replace-JS_STATIC_ASSERT-outside-js-directory.patch → part-2-replace-JS_ASSERT-outside-js-directory.patch
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.
Whiteboard: [leave open]
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
er sorry, "in part 3" (not part 2)
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1074911
You need to log in before you can comment on or make changes to this bug.