-
Notifications
You must be signed in to change notification settings - Fork 67
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
Remove [[ExecutionContext]] #320
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch looks like a correct way to implement @syg's suggestion, but I don't understand the motivation for the change. Stashing an execution context is a sort of common technique to make it easier to get into a Realm, e.g., also found in HTML's environment settings objects.
My only goal is unblock all the way to Stage 4, so I don't have a strong preference for this PR or otherwise. Somehow this proposed approach makes I'm also pretty confident we're going to eventually rename the |
Pinging @syg as I want to make sure this address his feedback. |
c04ae1e
to
57d8f77
Compare
PR rebased with conflicts resolved. |
pinging the @tc39/ecma262-editors here. This PR has no observable changes, but matches a request from @syg on the Stage 3 review. I'd like to confirm this is the desired direction for the eventual PR to ECMA-262. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense.
57d8f77
to
a7566c4
Compare
@ptomato is this ready for review? what is the latest here? |
@caridy I rebased it the other day, but I don't know the context of the original request, so maybe @leobalter or @syg should have a look before merging it. |
maybe @nicolo-ribaudo has opinions about this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to me, I actually prefer the new approach of creating a fresh EC every time rather than just creating fresh environment records to attach to it.
a7566c4
to
d9176a8
Compare
Unfortunately this conflicted with #409 merged soon after your review @nicolo-ribaudo. I've rebased it and fixed the conflicts. Should be good to go, but I'd appreciate if you took another quick look at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc @syg @caridy @rwaldron