Closed
Bug 1270076
Opened 9 years ago
Closed 8 years ago
Do not use enumerated types to debug CallArgs
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: nox, Assigned: Waldo)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
nox
:
review+
|
Details | Diff | Splinter Review |
Enumerated types are very hard to bind to other languages (especially Rust), let's avoid them in that very important public API.
Comment 1•9 years ago
|
||
Comment on attachment 8748602 [details] [diff] [review]
0001-Do-not-use-enumerated-types-to-debug-CallArgs.patch
Review of attachment 8748602 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/public/CallArgs.h
@@ +123,4 @@
> mutable bool usedRval_;
> void setUsedRval() const { usedRval_ = true; }
> void clearUsedRval() const { usedRval_ = false; }
> +#endif
I understand that the symbol are loaded from the parent class when we build with an optimized build.
Still, I would prefer to have a #else which redefine the empty function in such case.
Changing the lookup is a bad habit, and C++ has some corner case around looking up symbols. This is not the case, but I do not want such thing to become usual.
@@ +236,3 @@
> private:
> + friend CallReceiverBase<IncludeUsedRval> JS::CallReceiverFromVp(Value *vp);
> + friend CallReceiverBase<IncludeUsedRval> JS::CallReceiverFromArgv(Value *argv);
From what you told me you are going to investigate a similar issue in mfbt (why clang fails to bind this name to the name of the class), if you find a work-around in clang, we should not change these lines either.
Attachment #8748602 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 2•9 years ago
|
||
Addressed :nbp's remarks.
Attachment #8748602 -
Attachment is obsolete: true
Attachment #8748602 -
Flags: review?(jwalden+bmo)
Attachment #8748627 -
Flags: review?(jwalden+bmo)
Reporter | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 3•9 years ago
|
||
nox, what exactly is hard to bind about this case? Is it that there's a template parameter that's an enum (I know Rust doesn't support type-parameters that are values), or something about the enum itself?
Also: what's going on? I don't know if we've ever talked. How are these bindings being generated, what are they for, and how can we help?
Assignee | ||
Comment 4•9 years ago
|
||
FYI, a fresh laptop arrived just today \o/ so I probably should be able to get to this tomorrow.
Assignee | ||
Comment 5•8 years ago
|
||
I bitrotted the original pretty good doing simplifications in this area, so I should probably redo it for politeness. :-) I think the original had some extra unnecessary complexity to it, as well, that this version was able to remove. Win!
Attachment #8759257 -
Flags: review?(nox)
Assignee | ||
Updated•8 years ago
|
Assignee: nox → jwalden+bmo
Assignee | ||
Updated•8 years ago
|
Attachment #8748627 -
Attachment is obsolete: true
Attachment #8748627 -
Flags: review?(jwalden+bmo)
Reporter | ||
Updated•8 years ago
|
Attachment #8759257 -
Flags: review?(nox) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74111fa444f
Make CallArgs's JS_DEBUG-only handling of asserting proper rval()/calleev() sequencing work without using enumerated types, for Servo bindings. r=nox/r=jwalden tag-team effort
Comment 8•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•