module: support for circular import #4551
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
obastemur
force-pushed
the
module_circular
branch
2 times, most recently
from
48082f2 to
8329c60
Compare
Collaborator
|
Definitely neater than my proposed fix was and does indeed fix the issue so I've closed my PR. My only comment would be that I'd like to see the test case added. |
jackhorton
reviewed
Jan 14, 2018
| const static uint InvalidSlotIndex = 0xffffffff; | ||
| // TODO: move non-GC fields out to avoid false reference? | ||
| // This is the parsed tree resulted from compilation. | ||
| // This is the parsed tree resulted from compilation. |
Contributor
There was a problem hiding this comment.
Is this comment supposed to be one line below now?
| if (moduleRecord->shouldGenerateRootFunction) | ||
| { | ||
| moduleRecord->shouldGenerateRootFunction = false; | ||
| moduleRecord->GenerateRootFunction(); |
Contributor
There was a problem hiding this comment.
Can GenerateRootFunction set the above flag false itself?
Collaborator
Author
There was a problem hiding this comment.
It's actually false rest of the times.
Collaborator
Author
@rhuanjl on your previous branch, could you please make test case a single commit? So, I could cherry pick here. |
Collaborator
|
@obastemur |
Contributor
|
Please add a native tests to cover the scenario |
obastemur
force-pushed
the
module_circular
branch
from
8329c60 to
4f1aefe
Compare
obastemur
force-pushed
the
module_circular
branch
from
4f1aefe to
aac4d94
Compare
boingoing
approved these changes
Jan 16, 2018
Contributor
boingoing
left a comment
boingoing
left a comment
There was a problem hiding this comment.
Looks good, thanks for fixing that memory leak which makes testing hard. 👍
|
|
||
| void SourceTextModuleRecord::GenerateRootFunction() | ||
| { | ||
| ScriptContext* scriptContext = GetScriptContext(); |
Contributor
There was a problem hiding this comment.
Make sense to assert this->WasDeclarationInitialized() ?
Collaborator
Author
There was a problem hiding this comment.
Sounds good, done!
Collaborator
Author
|
@rhuanjl @jackhorton @akroshg @boingoing Thanks for review! |
chakrabot
pushed a commit
that referenced
this pull request
Jan 16, 2018
Merge pull request #4551 from obastemur:module_circular Alternative to #4550 , fixes #4482 Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support. I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl #### how it works Separate `ModuleDeclarationInstantiation` into `ModuleDeclarationInstantiation` and `GenerateRootFunction`. This way, in case `childrenModuleSet` has circular dependents, we may instantiate all the modules prior to triggering the rest
chakrabot
pushed a commit
that referenced
this pull request
Jan 16, 2018
Merge pull request #4551 from obastemur:module_circular Alternative to #4550 , fixes #4482 Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support. I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl #### how it works Separate `ModuleDeclarationInstantiation` into `ModuleDeclarationInstantiation` and `GenerateRootFunction`. This way, in case `childrenModuleSet` has circular dependents, we may instantiate all the modules prior to triggering the rest
chakrabot
pushed a commit
that referenced
this pull request
Jan 17, 2018
…ircular import Merge pull request #4551 from obastemur:module_circular Alternative to #4550 , fixes #4482 Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support. I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl #### how it works Separate `ModuleDeclarationInstantiation` into `ModuleDeclarationInstantiation` and `GenerateRootFunction`. This way, in case `childrenModuleSet` has circular dependents, we may instantiate all the modules prior to triggering the rest
This was referenced Jan 19, 2018
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #4550 , fixes #4482
Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support.
I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl
how it works
Separate
ModuleDeclarationInstantiationintoModuleDeclarationInstantiationandGenerateRootFunction. This way, in casechildrenModuleSethas circular dependents, we may instantiate all the modules prior to triggering the rest