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

Editorial: extract StringNumericValue from MV and add/use RoundStringMVResult helper #2435

Merged
merged 1 commit into from
Jul 10, 2021
Merged

Editorial: extract StringNumericValue from MV and add/use RoundStringMVResult helper #2435

merged 1 commit into from
Jul 10, 2021

Conversation

michaelficarra
Copy link
Member

No description provided.

@michaelficarra michaelficarra requested a review from a team June 14, 2021 21:10
@bakkot
Copy link
Contributor

bakkot commented Jun 14, 2021

Note that this races with #1554.

@michaelficarra michaelficarra mentioned this pull request Jun 15, 2021
Merged
jmdyck
jmdyck previously requested changes Jun 16, 2021
View reviewed changes
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@michaelficarra
Copy link
Member Author

@jmdyck Thanks for the review. I've made another major refactoring to split this AO from MV. Please review again.

@michaelficarra michaelficarra changed the title Editorial: refactor runtime MV and add/use RoundStringMVResult Editorial: extract StringNumericValue from MV and add/use RoundStringMVResult helper Jun 16, 2021
jmdyck
jmdyck reviewed Jun 19, 2021
View reviewed changes
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@jmdyck
Copy link
Collaborator

jmdyck commented Jun 19, 2021

(Hm, that's weird. I meant to submit a reply and a review separately, but GitHub seems to have merged them. I think it still makes sense.)

@ljharb ljharb requested a review from jmdyck June 23, 2021 21:49
syg
syg approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm, though I'd like to hear the reasoning for not expanding SV, TV, and TRV in prose.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
jmdyck
jmdyck reviewed Jun 24, 2021
View reviewed changes
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

I believe this PR is now semantics-preserving.

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@jmdyck
Copy link
Collaborator

jmdyck commented Jun 24, 2021

(Hrmph. GitHub took my comment about _n_ as continuing a resolved conversation, rather than starting a new one. I don't see any way to fix it after the fact.)

@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
bakkot
bakkot reviewed Jul 5, 2021
View reviewed changes
spec.html Outdated Show resolved Hide resolved
@bakkot bakkot added the editor call to be discussed in the next editor call label Jul 5, 2021
bakkot
bakkot approved these changes Jul 5, 2021
View reviewed changes
Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

This LGTM other than the outstanding comments.

bakkot
bakkot reviewed Jul 7, 2021
View reviewed changes
spec.html Outdated Show resolved Hide resolved
@michaelficarra michaelficarra mentioned this pull request Jul 7, 2021
Merged
@michaelficarra
Copy link
Member Author

This is ready for review again.

@michaelficarra michaelficarra added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Jul 7, 2021
@ljharb
Copy link
Member

ljharb commented Jul 7, 2021

@michaelficarra should i wait to merge this until there's a stamp after your latest comment, or is it good to go now?

@bakkot
Copy link
Contributor

bakkot commented Jul 7, 2021

@ljharb It's good to go.

@michaelficarra @ljharb
Editorial: refactor runtime MV and add/use RoundStringMVResult (#2435)
47568ca 
reword some things that read like initialisms

fix linting error

accept some changes

extract StringNumericValue from MV on the string grammar

use StringNumericValue

further describe "the mathematical value denoted by"

address review comments

don't round NonDecimalIntegerLiteral
@ljharb ljharb dismissed jmdyck’s stale review July 10, 2021 20:51

comments addressed

@ljharb ljharb merged commit 47568ca into master Jul 10, 2021
@ljharb ljharb deleted the mv branch July 10, 2021 20:56
@anba anba mentioned this pull request Jul 23, 2021
Closed
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
@michaelficarra @mathiasbynens
Editorial: refactor runtime MV and add/use RoundStringMVResult (tc39#…
b6df42a 
…2435)

reword some things that read like initialisms

fix linting error

accept some changes

extract StringNumericValue from MV on the string grammar

use StringNumericValue

further describe "the mathematical value denoted by"

address review comments

don't round NonDecimalIntegerLiteral
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmdyck jmdyck jmdyck left review comments

@bakkot bakkot bakkot approved these changes

@ljharb ljharb ljharb left review comments

@syg syg syg approved these changes

Assignees
No one assigned
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michaelficarra @bakkot @jmdyck @ljharb @syg