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

Editorial: be explicit about parsing realms #275

Closed
wants to merge 1 commit into from

Conversation

domenic
Copy link
Member

@domenic domenic commented Dec 22, 2015

When creating script and module records via ParseScript/ParseModule, currently we always use the running execution context's Realm. This pushes the dependency on the current execution context out a level, to the top-level jobs, ensuring that the parsing operations do not depend on implicit global state and can be used without a running execution context.

This is not a big deal, but as explained above it seems like an improvement. Incidentally (not primarily), it allows HTML to avoid the awkward dance of:

  1. Look up realm execution context, an execution context corresponding to the desired Realm realm.
  2. Push realm execution context onto the execution context stack.
  3. Let s be ParseScript(sourceText, hostDefined).
  4. Remove realm execution context from the execution context stack

in favor of the simpler

  1. Let s be ParseScript(sourceText, realm, hostDefined).

I labeled this as "editorial" instead of "layering" since IMO it's a solid editorial change independent of incidental layering benefits. "Layering" seems more for changes that are necessary for layering work.


An alternate design would be to not pass realm to ParseScript/ParseModule at all and instead have their callers (whether the callers be HTML or ScriptEvaluationJob/TopLevelModuleEvaluationJob) set returnedScript.[[Realm]]. The same goes for hostDefined actually. But it seems kind of nicer to have ParseScript/ParseModule return fully-formed records.

When creating script and module records via ParseScript/ParseModule, currently we always use the running execution context's Realm. This pushes the dependency on the current execution context out a level, to the top-level jobs, ensuring that the parsing operations do not depend on implicit global state and can be used without a running execution context.
@bterlson
Copy link
Member

Committed as dfdd127.

@bterlson bterlson closed this Dec 27, 2015
@UltCombo
Copy link
Contributor

@bterlson By the way, you can post the full commit hash. GitHub's markdown will automatically shorten and link it.

@abdulhannanali
Copy link

@ljharb sorry! deleted the inappropriate reply...

@abdulhannanali
Copy link

@UltCombo Sorry to you too

@bterlson
Copy link
Member

@UltCombo Usually the short hash is more convenient for me. GitHub would link it, but I probably didn't get the commits pushed. The plane wifi was spotty :/ Will push the missing commits soon!

@UltCombo
Copy link
Contributor

@bterlson Oh ok, I thought you were shortening them manually. In case you prefer and didn't know yet, you may add "Closes #ISSUE_NUMBER" to the commit message (body or subject) when pulling in the commit and GitHub will automatically close the issue and link to the commit.

@abdulhannanali The whole point of GitHub is being open. More specifically, contributing and helping each other. I'm not trying to be rude, just sharing information that might be useful to someone else. And FYI, followers and contribution graphs are very artificial metrics, which mean nothing to me. I do respect @bterlson, not for such metrics but rather for his long years in the industry and the arduous work that is the editorship of the ECMA-262 specification.

@UltCombo
Copy link
Contributor

By the way, @abdulhannanali I didn't take offence from your comment. I wrote the text above because I believe it may be enlightening to some people (not specifically you, but other repository watchers as well).

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