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.

Sign up for GitHub

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

Jump to bottom

misc editorial #452

Closed
wants to merge 18 commits into from
Closed

misc editorial #452

wants to merge 18 commits into from

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Mar 3, 2016

No description provided.

@jmdyck jmdyck changed the title Add underscores to 3 var names from recent commit misc editorial Mar 4, 2016
@bterlson
Copy link
Member

Great finds!

What do you think about using emu-val (emd * syntax) for number values (like string and Boolean values)?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Mar 19, 2016

I certainly think we should use * for the 'special' Number values (+/-0, +/-inifinity, and NaN). But for 'ordinary' numeric literals, I'm not sure. In the MV rules, literals should be interpreted as mathematical values rather than Number values, so it would be misleading/confusing to use * there. There might be other such cases.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Mar 19, 2016

Also, if you do decide to add * to numeric literals (that denote Number values), note that you'd need to treat 0 specially, because *0* isn't a Number value per se. Presumably, you'd need to make it *+0*.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Mar 19, 2016

Anyhow, I'd prefer that that issue not delay acceptance of this PR. (I've got another one in the wings that's waiting for this one to be merged.)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Mar 24, 2016

(just rebased to upstream master)

@jmdyck jmdyck mentioned this pull request Mar 26, 2016
Closed
jmdyck added 18 commits April 15, 2016 16:47
@jmdyck
Add underscores to 3 var names from recent commit
6bf01ab
@jmdyck
ED: Insert comma when "Else" is followed by a verb
a607e76 
... (e.g. "let", "set", "return")
@jmdyck
ED: Delete comma from "Else, if"
9db5c00 
... because the comma-less version is much more common in the spec.
@jmdyck
ED: Consistify commas in "Else <condition>"
c9915d8 
The 'spec consensus' seems to be that
there should be a comma after the condition,
and not after the "Else".
@jmdyck
Delete apostrophe-s from "Runtime Semantics: MV's"
4167c6d 
(Other similar section titles don't have apostrophe-s.)
@jmdyck
Drop a few unnecessary uses of <emu-eqn>
82b9538
@jmdyck
Change null to *null*
8b9d598 
... because it's talking about the result of calling [[GetPrototypeOf]].
@jmdyck
Remove spurious backticks from 1 and -1
e0e8f85
@jmdyck
Move OrdinaryToPrimitive() to its own clause
99b4439
@jmdyck
Simplify ValidateAndApplyPropertyDescriptor defn
42067a5 
Drop the introductory paragraph that alters the interpretation of the
algorithm (but not in a way that makes it obvious which steps are affected),
and instead just tweak the two steps that are affected (as far as I can tell),
to make things explicit.
@jmdyck
Repair the preamble for CreateIntrinsics()
e746f07
@jmdyck
Delete spurious "is" in preamble for GetViewValue
75c9321
@jmdyck
CreateBuiltinFunction: reorder params in preamble
8b814d0 
... to match order in header.
@jmdyck
Repair a non-sentence in the preamble for UTC()
0575786
@jmdyck
Insert a couple commas
8e97ec5 
... into the preambles of AllocateTypedArray and TriggerPromiseReactions
@jmdyck
Capitalize the first word after "Assert:"
fde05ea 
... (assuming it isn't a variable name).
The spec was somewhat divided on this point,
but it's capitalized in the majority of cases,
so go with that.
@jmdyck
Add a period at end of (recently added) step
4701486
@jmdyck
Append "then" to If-step-with-substeps
407f8c2 
(6 occurrences from a recent commit)
@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 15, 2016

@bterlson: This PR has been open for 6 weeks now. Is there something preventing it from being merged?

@bterlson
Copy link
Member

Just review time. I will get to it today I promise.

(Sorry, of late I've been treating PRs like a LIFO queue, which is probably a bad policy)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 15, 2016

Thanks!

@bterlson
Copy link
Member

Please accept this as a token of my regret: 🍁

bterlson
bterlson reviewed Apr 16, 2016
View reviewed changes
spec.html
@@ -4002,7 +4002,7 @@
1. If the _LeftFirst_ flag is *true*, then
1. Let _px_ be ? ToPrimitive(_x_, hint Number).
1. Let _py_ be ? ToPrimitive(_y_, hint Number).
1. Else the order of evaluation needs to be reversed to preserve left to right evaluation
1. Else the order of evaluation needs to be reversed to preserve left to right evaluation,
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is not actually a condition but an assert and so would argue weakly for "Else, the order of evaluation..." instead. But this is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's both a condition and an assertion. That is, every occurrence of

    Else <condition>,
        ...

is basically equivalent to:

    Else,
        Assert: <condition>
        ...

@bterlson
Copy link
Member

Looks good. Rebasing, cleaning up commit messages and merging. Thanks, and sorry again for the delay.

@bterlson bterlson closed this in b313823 Apr 16, 2016
@jmdyck jmdyck deleted the editorial branch April 16, 2016 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jmdyck @bterlson