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

Simplify the definition of the "entry" concept #2499

Merged
merged 2 commits into from
Apr 18, 2017
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 3, 2017

It turns out that the "entrance counter" concept was unnecessary.
Whenever we run script, we increment it, and whenever we clean up after
running script, we decrement it. So it is always nonzero. Thus we can
simply look for whether it's a realm execution context.

Fixes #2441.

/cc @jensl @bzbarsky. This should be effectively an editorial change.

It turns out that the "entrance counter" concept was unnecessary.
Whenever we run script, we increment it, and whenever we clean up after
running script, we decrement it. So it is always nonzero. Thus we can
simply look for whether it's a realm execution context.

Fixes #2441.
source Outdated
@@ -120075,6 +120069,7 @@ INSERT INTERFACES HERE
Jens Fendler,
Jens Lindström,
Copy link

Choose a reason for hiding this comment

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

"Jens Lindström" used to be me, before I got married. Might make sense to remove this entry, now that my new name was added to the list.

Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

I discussed this with @jensl to try to understand what's going on here. Since the counter is unused it can be removed without any normative change. Having it there opens the opportunity for bugs of using the wrong realm. No new tests needed for this change, but having tests in general for multiple globals/environment settings objects/etc would be good.

@domenic
Copy link
Member Author

domenic commented Apr 18, 2017

Thanks @zcorpan. We indeed keep adding new tests, mostly while checking off boxes in #1430 and #1431.

@domenic domenic merged commit 36d771a into master Apr 18, 2017
@domenic domenic deleted the simplify-entry branch April 18, 2017 13:50
@bzbarsky
Copy link
Contributor

Looks good to me. Sorry, I thought I had said that earlier....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants