Closed Bug 1168500 Opened 6 years ago Closed 6 years ago

Remove the operator,

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nbp, Assigned: nbp)

Details

Attachments

(1 file)

For some unexplained reasons, a lot of persons are upset when they learn that we overload the operator comma in Gecko. Many of them suggest that the author of such code should be fired (burn to death?).

Apparently, even after 3 years, people are still not getting used to this code, and as I care for my life, I suggest that we replace these operator comma by some variadic templates :)
Attachment #8610716 - Flags: review?(jwalden+bmo)
Comment on attachment 8610716 [details] [diff] [review]
replace-operator-comma.patch

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

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +665,1 @@
>  }

I guess we'd probably better use `ArgList(ArgTypes&&... args)` and `return ArgSeq<ArgTypes...>(mozilla::Forward(args)...);` here?
Comment on attachment 8610716 [details] [diff] [review]
replace-operator-comma.patch

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

Sorry about comment 1. There was probably some bug which makes it display only one line before.

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +658,5 @@
>  };
>  
> +template <typename... ArgTypes>
> +inline ArgSeq<ArgTypes...>
> +ArgList(ArgTypes... args)

I meant this line and

@@ +663,2 @@
>  {
> +    return ArgSeq<ArgTypes...>(args...);

this line.
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #1)
> I guess we'd probably better use `ArgList(ArgTypes&&... args)` and `return
> ArgSeq<ArgTypes...>(mozilla::Forward(args)...);` here?

I agree that this would be nice for immediates, but this would not work for registers (as opposed to the example in the comment, the registers are likely to be used more than once).

Also, this does not matter much as most of these structures are quite small.
Comment on attachment 8610716 [details] [diff] [review]
replace-operator-comma.patch

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

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +615,5 @@
> +// this function is an instance of a class which provides a "generate" in charge
> +// of pushing the argument, with "pushArg", for a VMFunction.
> +//
> +// Such list of arguments can be created by using the "ArgList" function which
> +// creates one instance of "ArgSeq", where the type of the arguments are infered

inferred

@@ +638,5 @@
>      }
>  };
>  
> +template <typename ArgType, typename... ArgTypes>
> +class ArgSeq<ArgType, ArgTypes...> : public ArgSeq<ArgTypes...>

Maybe rename ArgType to HeadArgType, ArgTypes to TailArgTypes?  Or FirstArgType and TrailingArgTypes, if you're not an ML sort of person.  (Don't even *think* about CarArgType/CdrArgTypes.  :-) )  Point is we want the names of the types to indicate their position, or at an absolute minimum not differ by only a single character.

@@ +646,3 @@
>  
>    public:
> +    ArgSeq(const ArgType& first, ArgTypes... last)

template <typename... TailArgTypes>
ArgSeq(const HeadArgType& first, TailArgTypes&&... tailArgs)

so that all the rvalue stuff boils away through the entire thing.  Just general consistency about forwarding things most efficiently, even if arg types are usually just words or something that pass efficiently by default.

@@ +652,2 @@
>  
> +    // Push the last argument first.

I'd make this

  // Arguments are pushed in reverse order, from last argument to first argument.

@@ +665,1 @@
>  }

Yeah, rvalue references and forwarding is preferable.

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #1)
> > I guess we'd probably better use `ArgList(ArgTypes&&... args)` and `return
> > ArgSeq<ArgTypes...>(mozilla::Forward(args)...);` here?
> 
> I agree that this would be nice for immediates, but this would not work for
> registers (as opposed to the example in the comment, the registers are
> likely to be used more than once).

That's not a problem -- you're saving the provided arguments into a series of |first_| fields.  Reusing the rvalue reference later would be a problem, but the idiom here doesn't do that.

> Also, this does not matter much as most of these structures are quite small.

This is just general argument-forwarding hygiene -- we should do it the right way all the time, so the "tricky" cases aren't unusual.
Attachment #8610716 - Flags: review?(jwalden+bmo) → review+
<sfink> Waldo: btw, my actual opinion would probably be either Head/Tail, or First/Rest

Rest is better than Trailing, yeah.  Given he suggested head/tail first, and I kind of maybe prefer that, let's go with that.
https://hg.mozilla.org/mozilla-central/rev/54be257c5313
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.