Closed
Bug 712873
Opened 11 years ago
Closed 7 years ago
Use the mfbt runtime assertion macros rather than JS aliases to them
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
DUPLICATE
of bug 1074911
People
(Reporter: Waldo, Assigned: Waldo)
Details
(Whiteboard: [leave open])
Attachments
(5 files)
1.28 MB,
patch
|
luke
:
review-
|
Details | Diff | Splinter Review |
18.53 KB,
patch
|
luke
:
review-
|
Details | Diff | Splinter Review |
8.28 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
76.86 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
8.24 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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 | ||
Comment 2•11 years ago
|
||
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)
![]() |
||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
Comment on attachment 583727 [details] [diff] [review] Convert JS_ASSERT{,_IF} No, thank you.
Attachment #583727 -
Flags: review?(luke) → review-
![]() |
||
Updated•11 years ago
|
Attachment #583728 -
Flags: review?(luke) → review-
![]() |
||
Comment 6•11 years ago
|
||
(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•8 years ago
|
||
Part 1: Replace JS_ASSERT with MOZ_ASSERT outside js/ directory.
Attachment #8377415 -
Flags: review?(luke)
Comment 8•8 years ago
|
||
Part 2: Replace JS_ASSERT with MOZ_ASSERT in js/ directories outside js/src/ directory.
Attachment #8377418 -
Flags: review?(luke)
Comment 9•8 years ago
|
||
Part 3: Replace JS_ALWAYS_TRUE with MOZ_ALWAYS_TRUE everywhere outside js/src/ directory.
Attachment #8377419 -
Flags: review?(luke)
![]() |
||
Updated•8 years ago
|
Attachment #8377415 -
Flags: review?(luke) → review+
![]() |
||
Updated•8 years ago
|
Attachment #8377418 -
Flags: review?(luke) → review+
![]() |
||
Comment 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8377418 -
Attachment description: part-2-replace-JS_STATIC_ASSERT-outside-js-directory.patch → part-2-replace-JS_ASSERT-outside-js-directory.patch
Comment 12•8 years ago
|
||
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]
Comment 13•8 years ago
|
||
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•8 years ago
|
||
er sorry, "in part 3" (not part 2)
Comment 15•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c952f7937ec1 https://hg.mozilla.org/mozilla-central/rev/28fa156efc31 https://hg.mozilla.org/mozilla-central/rev/d0259d0c1882
Assignee | ||
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1074911
You need to log in
before you can comment on or make changes to this bug.
Description
•