1585196 - Cleanup LazyScriptCreationData

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Review bot noted (after I had landed) that std::move is not necessary

We can remove them.

As well, LazyScriptCreationData can be changed to use field initializers

Comment 1

[Alok]

Is removing std::move the only pending task here? I see that LazyScriptCreationData has already been updated to use field initializers.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

It's deeply confusing, because there are two different "field initializers" in flight here.

What's already there is the inside-the-JS-engine notion of a "field initializer", related to the TC39 fields proposal.

What I wanted to be changed was instead to use Default Initializers for Member Variables, which I'd originally had described to me as 'field initializers' -- Today I learned that was the wrong name!

Hopefully that clarifies things.

Comment 3

[Alok]

Ah! So, I presume, you want these changed

• Remove std::move
• Change the default constructor to only initialize innerFunctionBoxes = cx and initialize the rest with the default initializers?

Something like this?

struct LazyScriptCreationData {
  frontend::AtomVector closedOverBindings;

  // This is traced by the functionbox which owns this LazyScriptCreationData
  FunctionBoxVector innerFunctionBoxes;
  bool strict = false;

  mozilla::Maybe<FieldInitializers> fieldInitializers;

  explicit LazyScriptCreationData(JSContext* cx)
      : innerFunctionBoxes(cx),
}

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Yeah, that's what I was thinking.

I had to do some experimentation to make sure that the default constructor would be run for more complex zero-arg constructor types like AtomVector, but this experiment suggests we'd be OK

#include <iostream>

int inits = 0;
struct I {
    I() { inits++; } 
};
 
struct S
{
    int n = 0;
    I i; 
    S() { }  
    S(int arg) : n(arg) { }
};
 
int main()
{
    S s1;
    S s2(7);
    std::cout << inits << "\n"; // Prints 2, as I's constructor runs twice. 
}

(Adapted from the default member initializer section at cppreference).

Comment 5

[Alok]

Attachment: Bug 1585196 - Cleanup LazyScriptCreationData, r=mgaudet

Comment 6

[Alok]

Makes sense. Submitted a patch.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6232b11cd533
Cleanup LazyScriptCreationData, r=mgaudet

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6232b11cd533