1270076 - Do not use enumerated types to debug CallArgs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Anthony Ramine [:nox]]

Attachment: 0001-Do-not-use-enumerated-types-to-debug-CallArgs.patch

Enumerated types are very hard to bind to other languages (especially Rust), let's avoid them in that very important public API.

Comment 1

[Nicolas B. Pierron [:nbp]]

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.

Comment 2

[Anthony Ramine [:nox]]

Attachment: 0001-Do-not-use-enumerated-types-to-debug-CallArgs.patch

Addressed :nbp's remarks.

Comment 3

[Jason Orendorff [:jorendorff]]

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?

Comment 4

[Jeff Walden [:Waldo]]

FYI, a fresh laptop arrived just today \o/ so I probably should be able to get to this tomorrow.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Alterna-patch

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!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e74111fa444f