More module tests and implement RegisterModuleSource #4582
Conversation
obastemur
requested review from
Penguinwizzard,
akroshg,
boingoing and
dilijev
jackhorton
left a comment
jackhorton
left a comment
There was a problem hiding this comment.
I am pretty sure that there are memory leaks in the new AutoString/FileNode/SourceMap code. Could it be altered to use RAII and const references and fewer pointers?
| { | ||
| if (!HostConfigFlags::flags.MuteHostErrorMsgIsEnabled) | ||
| // check if have it registered | ||
| AutoString *data; |
There was a problem hiding this comment.
Since data is a pointer, I don't think the AutoString destructor will run -- is that an issue?
There was a problem hiding this comment.
It shouldn't run by design and intention. See constructor. It takes over the ownership (there is a comment). Source code map lifetime == process lifetime.
obastemur
force-pushed
the
module_register__
branch
from
a688afc to
3b79e4d
Compare
Could you please point them out? |
|
So I think the part I am considering leaking is https://github.com/Microsoft/ChakraCore/pull/4582/files#diff-05783da3b8bba4fa92445ddb47da7d51R309, where we |
jackhorton
left a comment
jackhorton
left a comment
There was a problem hiding this comment.
Altering changes requested to a comment.
obastemur
force-pushed
the
module_register__
branch
from
3b79e4d to
7eb6e1c
Compare
obastemur
force-pushed
the
module_register__
branch
from
7eb6e1c to
a782b4e
Compare
|
Will the idea in time to be to move most tests out of the separate files and into the combined files with this new API? Seems great from the perspective of making the tests easier to update and keep track of. My only concern would be if we dropped all of the separate tests the file loading routines would stop getting tested - this may be a ch only issue and not a chakracore issue but probably best to keep them intact. |
|
@rhuanjl there is no greater plan here. I just saw an opportunity to use tests from our internal testing tool and implemented the interface. ++ moved the tests into sep. folder. I wouldn't change any of the existing tests but going forward we better use the new interface instead of adding more files. |
Merge pull request #4583 from rhuanjl:moduleAgain This re-fixes #4482 Notes: 1. Test case was previously dependent on load order. Load order was changed which made the issue appear to be fixed (and test pass) when it wasn't actually fixed. - Test case revised to trigger issue irrespective of load order. 2. Have to complete all ModuleDeclarationInstantiation before we generate any root functions to ensure this issue can never occur. Conscious the test case may need moving to fit with #4582
Merge pull request #4583 from rhuanjl:moduleAgain This re-fixes #4482 Notes: 1. Test case was previously dependent on load order. Load order was changed which made the issue appear to be fixed (and test pass) when it wasn't actually fixed. - Test case revised to trigger issue irrespective of load order. 2. Have to complete all ModuleDeclarationInstantiation before we generate any root functions to ensure this issue can never occur. Conscious the test case may need moving to fit with #4582
…ircular children Merge pull request #4583 from rhuanjl:moduleAgain This re-fixes #4482 Notes: 1. Test case was previously dependent on load order. Load order was changed which made the issue appear to be fixed (and test pass) when it wasn't actually fixed. - Test case revised to trigger issue irrespective of load order. 2. Have to complete all ModuleDeclarationInstantiation before we generate any root functions to ensure this issue can never occur. Conscious the test case may need moving to fit with #4582
4 participants
Fixes #4577