Closed Bug 1257077 Opened 8 years ago Closed 8 years ago

Implement js::Fixed{Invoke,Construct}Args and js::Any{Invoke,Construct}Args as invoke/construct argument base classes

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(1 file)

Bunches of call/construct instances in C++ use a fixed number of arguments.  There's no reason they should be paying GCVector allocation costs.  Add some extra classes to support this, plus a couple more to serve as base classes for all invoke args, and for all construct args.
Attached patch PatchSplinter Review
Attachment #8731046 - Flags: review?(efaustbmo)
Comment on attachment 8731046 [details] [diff] [review]
Patch

Review of attachment 8731046 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/vm/Interpreter.cpp
@@ +599,5 @@
>                MutableHandleObject objp)
>  {
> +    // Explicitly qualify to bypass AnyConstructArgs's deliberate shadowing.
> +    args.CallArgs::setCallee(fval);
> +    args.CallArgs::setThis(MagicValue(JS_IS_CONSTRUCTING));

Yeah, this is a little ugly, but I understand why delete is important here. I wonder if we will have to recant on this, for the rest of browser ergonomics. If not, great!

::: js/src/vm/Stack.h
@@ +955,4 @@
>  
> +  public:
> +    bool init(unsigned argc) {
> +        MOZ_ASSERT(2 + argc + Construct > argc);  // no overflow

Not your fault. I actually changed this assert last, I think, but can we add a comment that's like
// Callee, This, Args, [new.target if constructing]
or some such? 2 is dotted around, but looks pretty magic, here.

@@ +991,5 @@
>  };
>  
> +/** Function call args of statically-known count. */
> +template <size_t N>
> +class FixedInvokeArgs : public detail::FixedArgsBase<false, N>

can we use MaybeConstruct, or something similar here, instead of hardcoding true and false? Seems like CONSTRUCT is clearer than true.
Attachment #8731046 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/4029d2aeb270
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.