Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Latest Esprima beta changes #595

Merged
merged 3 commits into from
Feb 22, 2019
Merged

Conversation

sam-lord
Copy link
Contributor

This update is a pre-requisite for adding arrow functions.

Unfortunately as I'm not completely familiar with the code / intentions behind all of the changes in Esprima, I may have gone about this in the wrong way (in particular I do recall reading that Esprima.Ast.List is not supposed to be exposed?).

I'd be happy to take any comments on board. Once this is in, the change to add arrow functions is very minor.

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good cleanup, thanks for taking the time! Could you check my comments about passing Esprima's List as ref due to it being a struct, otherwise it gets copied every time.

Jint/Engine.cs Outdated Show resolved Hide resolved
Jint/Engine.cs Outdated Show resolved Hide resolved
Jint/Runtime/Environments/DeclarativeEnvironmentRecord.cs Outdated Show resolved Hide resolved
Jint/Runtime/Interpreter/JintStatementList.cs Show resolved Hide resolved
Jint/Runtime/Interpreter/Statements/JintSwitchBlock.cs Outdated Show resolved Hide resolved
@Happypig375
Copy link

I think using the in modifier instead of ref would be better since it guarantees the list cannot be reassigned.

@lahma
Copy link
Collaborator

lahma commented Feb 17, 2019

But as Esprima list is not read-only I believe a defensive copy would be made.

@sam-lord
Copy link
Contributor Author

I agree with @lahma. I've just been reading this blog post to get a better grasp on the in modifier in C#: https://blogs.msdn.microsoft.com/seteplia/2018/03/07/the-in-modifier-and-the-readonly-structs-in-c/ - which concludes that a defensive copy will be made for structs which are not read-only.

I'll go ahead and update this PR with ref keywords.

Copy link
Contributor Author

@sam-lord sam-lord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of in modifier

@sam-lord
Copy link
Contributor Author

Is there anything stopping this from being merged in?

@lahma
Copy link
Collaborator

lahma commented Feb 22, 2019

Sorry to keep you waiting. @sebastienros makes the final call and I believe he's been a bit under the weather, hopefully back soon to review.

@sebastienros sebastienros merged commit 7b485ec into sebastienros:dev Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants