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: Add BigInt to the list of reference base value types and the language types intro text #1773

Merged
merged 1 commit into from Nov 19, 2019

Conversation

@bathos
Copy link
Contributor

bathos commented Nov 11, 2019

Similar to #1769 — BigInt was absent from the list of types that the base value component of a Reference could be.

Note that it is already included and handled as a possible base value in the HasPrimitiveBase abstract op.

I was unsure if there was any particular logic to the ordering of types, so I just slotted it in the ‘most alphabetic’ seeming part of the list. Alternatively, the sentence could be reworded to avoid listing them all if it’s felt that this is becoming an update hazard, e.g.

The base value component is either an Environment Record or a language type other than Null.

Or, if it’s desired to draw additional attention to undefined’s distinct significance here:

The base value component is either an Environment Record or a language type other than Null (but including undefined).

(ref #1515)

@claudepache

This comment has been minimized.

Copy link
Contributor

claudepache commented Nov 11, 2019

Alternatively, this could be reworded to avoid listing them all, e.g.

The base value component is either an Environment Record or a language type other than Null.

undefined plays a special role which could have better been modelled as empty. IMO, a better rewording would be:

The base value component is either undefined, or an ECMAScript language type other than Undefined and Null, or an Environment Record.

EDIT: I meant, of course:

The base value component is either undefined, or an ECMAScript language value other than undefined and null, or an Environment Record.

@claudepache

This comment has been minimized.

Copy link
Contributor

claudepache commented Nov 11, 2019

BTW, another place where BigInt is still missing, is in the list of types found in the introductory paragraph of Section 6.1 ECMAScript language type.

@bathos

This comment has been minimized.

Copy link
Contributor Author

bathos commented Nov 11, 2019

Thanks @claudepache, I’ve added the missing one you mentioned. For now I’ve left the full enumeration in the base value list to err on the side of minimal change, but will update it if it turns out there’s general agreement that a briefer version would be better.

@bathos bathos changed the title Editorial: Add BigInt to the list of possible reference base value types Editorial: Add BigInt to the lists of reference base value types and the language types intro text Nov 11, 2019
@bathos bathos changed the title Editorial: Add BigInt to the lists of reference base value types and the language types intro text Editorial: Add BigInt to the list of reference base value types and the language types intro text Nov 11, 2019
@ljharb ljharb requested review from syg, michaelficarra, littledan, bakkot, caiolima and tc39/ecma262-editors Nov 12, 2019
@ljharb
ljharb approved these changes Nov 12, 2019
Copy link
Member

ljharb left a comment

Let's keep this fix focused, and leave larger refactors to a followup PR.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@ljharb ljharb self-assigned this Nov 12, 2019
@bakkot

This comment has been minimized.

Copy link
Contributor

bakkot commented Nov 12, 2019

It is also missing from the first step of CreateListFromArrayLike. I've checked the rest of the spec and found no other omissions.

@bakkot
bakkot approved these changes Nov 12, 2019
@caiolima

This comment has been minimized.

Copy link
Contributor

caiolima commented Nov 12, 2019

Thank you very much for this PR!

@syg
syg approved these changes Nov 19, 2019
…pes (#1773)
@ljharb ljharb force-pushed the bathos:bigint-reference-base branch from 935ab9c to 788736c Nov 19, 2019
@ljharb ljharb merged commit 788736c into tc39:master Nov 19, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/ecma262-snapshots/deploy-preview Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.