Closed
Bug 1483275
Opened 6 years ago
Closed 6 years ago
Fix more SpiderMonkey unified build conflicts
Categories
(Core :: JavaScript Engine, enhancement, P5)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: tcampbell, Assigned: tcampbell)
References
Details
Attachments
(3 files, 2 obsolete files)
19.87 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
3.56 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
17.44 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
Fix some more unified build problems in js/src. This still doesn't fix |FILES_PER_UNIFIED_FILE = 1000|, but it gets us closer.
Assignee | ||
Comment 1•6 years ago
|
||
Small tweaks to prevent things from colliding in unified builds. - De-duplicate ReportExceptionClosure - Rename IsIdentifier - Other boring changes
Attachment #8999985 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•6 years ago
|
||
Move duplicated VMFunctions to jit/VMFunctions.cpp
Attachment #8999986 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•6 years ago
|
||
Share SimpleArithOperand. This technically changes semantics, but it seems minor and the right call.
Attachment #8999987 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•6 years ago
|
||
Some remaining problems: - gc::AbortReason / jit::AbortReason conflict. Bug 1483269 - LIR defines ToInt32, IsConstant, etc which are all a pain.. - ??? This clears my patch queue on this matter and I'll let anyone else do more follow-up in another bug if they desire.
Comment 5•6 years ago
|
||
Comment on attachment 8999985 [details] [diff] [review] 0001-Bug-1483275-Fix-some-SpiderMonkey-unified-build-conf.patch Review of attachment 8999985 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/intl/IntlObject.cpp @@ -30,5 @@ > #include "vm/JSObject-inl.h" > > using namespace js; > > -using mozilla::Range; I'm not particularly happy about this removal. It seems entirely fine/good to me for Range to be a thing using'd in individual files that are distinct, and I don't really like gunking up totally non-JIT files just for this. But, uh, I guess. (The same is also somewhat true for IsIdentifier -- but also there those are relatively fairly deemed "impl" functions, so the rename is justified in its own rights.) ::: js/src/irregexp/RegExpInterpreter.cpp @@ +141,5 @@ > } > for (size_t i = 0; i < (size_t) numRegisters; i++) > registers[i] = -1; > > +#define BYTECODE(name) case BC_##name: Honestly, this seems dumb and I would just replace it with actually typing out the expansion. IMO. ::: js/src/vm/ErrorReporting.h @@ +69,5 @@ > void throwError(JSContext* cx); > }; > > +class MOZ_STACK_CLASS ReportExceptionClosure > + : public ScriptEnvironmentPreparer::Closure Save for the inheritance bit, IMO this really should just go in JSContext.h with an inline definition for operator(). But I guess that would require putting SEP into its own public header or something, and that requires figuring out what a *real* API for that thing is (docs in jsfriendapi.h are about as sparse as you would unfortunately expect), so for now okay. (Also if REC can be final that would be a nice driveby improvement.) @@ +78,5 @@ > + > + bool operator()(JSContext* cx) override; > + > + private: > + JS::HandleValue exn_; Wasn't style to put these at top? (double-check me, I don't know I've internalized the rule well enough myself yet) ::: js/src/vm/RegExpObject.cpp @@ +329,5 @@ > return c == '\n' || c == '\r'; > } > > static MOZ_ALWAYS_INLINE bool > +IsRegExpLineTerminator(const char16_t c) Perhaps in a followup patch these two functions should die and we should define them in util/Unicode.h instead, removing the presumably-conflicting frontend/ ones? Poke r?me on that patch please.
Attachment #8999985 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 6•6 years ago
|
||
Carrying r=waldo from Comment 5. - Leave |using mozilla::Range| alone. Probably doesn't seem to actually come up any more and change should have been on JIT side anyways. - Expand BYTECODE macro - Address ReportExceptionClosure nits.
Attachment #8999985 -
Attachment is obsolete: true
Comment 7•6 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #6) > - Expand BYTECODE macro Fair warning: Whenever I get back to bug 1367105, I'll 100% revert this change to reduce our local changes compared to upstream irregexp. :-)
Comment 8•6 years ago
|
||
Ugh, I didn't notice that was an import. Don't bother with expanding BYTECODE except to move it, I guess.
Assignee | ||
Comment 9•6 years ago
|
||
Carrying r=waldo from Comment 3. I should have seen that coming.. reducing delta on irregexp to just |#undef BYTECODE| which Andre may end up reverting on me anyways...
Attachment #9000066 -
Attachment is obsolete: true
Attachment #9000076 -
Flags: review+
Updated•6 years ago
|
Attachment #8999986 -
Flags: review?(jdemooij) → review+
Comment 10•6 years ago
|
||
Comment on attachment 8999987 [details] [diff] [review] 0003-Bug-1483275-Use-one-definition-of-js-jit-SimpleArith.patch Review of attachment 8999987 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #8999987 -
Flags: review?(jdemooij) → review+
Comment 11•6 years ago
|
||
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5e7017ffdc Fix some SpiderMonkey unified-build conflicts. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/c13d9e177361 Remove duplicate VMFunction definitions. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f5d094aa19 Use one definition of js::jit::SimpleArithOperand. r=jandem
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cf5e7017ffdc https://hg.mozilla.org/mozilla-central/rev/c13d9e177361 https://hg.mozilla.org/mozilla-central/rev/a5f5d094aa19
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•