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: Make Reference type a Record value #2085

Merged
merged 1 commit into from
Oct 28, 2020
Merged

Editorial: Make Reference type a Record value #2085

merged 1 commit into from
Oct 28, 2020

Conversation

shvaikalesh
Copy link
Member

@shvaikalesh shvaikalesh commented Jul 8, 2020 •
edited
Loading

Resolves #333.

This PR:

  • makes Reference type a Record, moving field description to a Reference Fields table;
  • makes [[Base]] of an unresolvable reference absent instead of undefined (nice improvement suggested by @claudepache);
  • replaces (currently inconsistent) prose like "base value component of V" with field access, removing a few now unnecessary helpers (GetBase, GetReferencedName, and IsStrictReference);
  • replaces Reference definitions ("a value of type Reference whose ... component ...") with record literals.

A few drive-by tweaks: earlier IsUnresolvableReference check, inline error, and typo fix.

@ljharb ljharb added editor call to be discussed in the next editor call editorial change and removed editor call to be discussed in the next editor call labels Jul 8, 2020
@ljharb ljharb requested review from syg, michaelficarra, ljharb, bakkot and a team July 8, 2020 21:39
michaelficarra
michaelficarra reviewed Jul 16, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
michaelficarra
michaelficarra reviewed Jul 16, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
michaelficarra
michaelficarra reviewed Jul 16, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
michaelficarra
michaelficarra reviewed Jul 16, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
@michaelficarra michaelficarra mentioned this pull request Jul 16, 2020
Open
syg
syg reviewed Aug 5, 2020
View reviewed changes
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 Outdated Show resolved Hide resolved
claudepache
claudepache reviewed Aug 6, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
claudepache
claudepache reviewed Aug 6, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
claudepache
claudepache reviewed Aug 6, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
@shvaikalesh shvaikalesh force-pushed the reference-as-record branch 2 times, most recently from 6cd7ab4 to 90238b3 Compare August 15, 2020 14:17
ljharb
ljharb reviewed Aug 21, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
bakkot
bakkot reviewed Aug 23, 2020
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.

There is a NOTE in ResolveBinding which still refers to "a Reference value with its referenced name component equal to [...]"; presumably that should be "a Reference Record whose [[ReferencedName]] is equal to [...]"

LGTM other than the comments I've left.

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 Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@bakkot bakkot force-pushed the reference-as-record branch from 1eda3be to c87dba0 Compare September 7, 2020 23:20
@bakkot
Copy link
Contributor

bakkot commented Sep 7, 2020

I rebased and pushed a couple of wording tweaks.

syg
syg reviewed Sep 14, 2020
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.

This is a great improvement and generally looks very good.

I'd like to see the nits below addressed as well as hear from the other editors on their thoughts on naming the sentinel value for [[Base]] ~unresolvable~ instead of ~empty~.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
syg
syg approved these changes Oct 28, 2020
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, this is a great PR.

spec.html Show resolved Hide resolved
@ljharb ljharb self-assigned this Oct 28, 2020
@ljharb ljharb force-pushed the reference-as-record branch from ba07e17 to 35a7d35 Compare October 28, 2020 22:42
@ljharb ljharb merged commit 35a7d35 into tc39:master Oct 28, 2020
@bakkot bakkot mentioned this pull request Nov 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claudepache claudepache claudepache left review comments

@bakkot bakkot bakkot approved these changes

@ljharb ljharb ljharb left review comments

@michaelficarra michaelficarra michaelficarra approved these changes

@syg syg syg approved these changes

Assignees

@ljharb ljharb

Labels
editorial change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rewrite References as Records
6 participants
@shvaikalesh @bakkot @ljharb @syg @michaelficarra @claudepache