Closed Bug 1585196 Opened 2 months ago Closed 2 months ago

Cleanup LazyScriptCreationData

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: mgaudet, Assigned: aloknnikhil, Mentored)

References

Details

(Whiteboard: [lang=c++] )

Attachments

(1 file)

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

Priority: -- → P3

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

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.

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),
}

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).

Makes sense. Submitted a patch.

Assignee: nobody → aloknnikhil
Keywords: checkin-needed

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6232b11cd533
Cleanup LazyScriptCreationData, r=mgaudet

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.