proposal-symbols-as-weakmap-keys icon indicating copy to clipboard operation
proposal-symbols-as-weakmap-keys copied to clipboard

Reviewers for Stage 3

Open leobalter opened this issue 3 years ago • 11 comments

Tracking issue for spec reviews for advancement to Stage 3.

From notes: JHD, RGN, BSH, ~~DE~~ MAH

  • [x] Richard Gibson @gibson042
  • [x] Bradford Smith @brad4d
  • [ ] Mathieu Hofman @mhofman
  • [x] Jordan Harband @ljharb
  • [ ] (Editor) Michael Ficarra @michaelficarra
  • [ ] (Editor) Kevin Gibbons @bakkot
  • [ ] (Editor) Shu-yu Guo @syg

leobalter avatar May 13 '21 23:05 leobalter

I'm still not a fan of the term "identity" here - I believe https://github.com/tc39/ecma262/issues/2405 specifically intends to only use that term with spec values and not ecmascript values.

Otherwise, the only nit I have is that HasIdentity could say "is Object or Symbol," and be reduced to two lines (none of these should be stage 3 blockers, but I'd strongly advocate for changing the word to something less confusing before merging into the main spec)

ljharb avatar May 14 '21 21:05 ljharb

LGTM for Stage 3. I think we can iterate on the names of abstract algorithms after Stage 3.

littledan avatar May 18 '21 00:05 littledan

LGTM for Stage 3 modulo concerns from @ljharb Thanks for pointing those out BTW.

brad4d avatar May 18 '21 23:05 brad4d

Thanks! I have no strong preference over the abstraction name. HasIdentity was called this way to reflect general functionality but I also don't intend to expand this proposal to other types.

One review item I received in private:

we can't have this be a slippery slope and add numbers and booleans to weak collections

I don't have intention or goals to add other primitive values to weak collections and this remains out of scope for this proposal. Someone else might wanna discuss about Records and Tuples but it's not the case for this proposal.

leobalter avatar May 18 '21 23:05 leobalter

Thanks! @ljharb @brad4d #18 should address most of it. I'm now using the most simple name for the abstraction and I hope we are good to go with it.

leobalter avatar May 19 '21 00:05 leobalter

Almost 1 year later, we are once again in a position where we require spec review as we prepare to go for stage 3.

As per #19 the full set of proposed changes is being done as a PR due to the proposal's need for many small changes (e.g. updating liveness to reference both objects and symbols).

https://github.com/acutmore/ecma262/pull/1

The key difference from last year is that 'registered symbols' (those created with Symbol.for) are not considered suitable weak keys/entries/targets.

cc: @mhofman @gibson042 @brad4d @ljharb @michaelficarra @bakkot @syg

acutmore avatar May 16 '22 21:05 acutmore

@acutmore Thanks for driving this! One pet peeve: we'd all benefit of a direct link to the rendered spec, but I'm also in favor of having this mentioned PR merged already for the said review.

leobalter avatar May 16 '22 22:05 leobalter

@acutmore Thanks for driving this! One pet peeve: we'd all benefit of a direct link to the rendered spec, but I'm also in favor of having this mentioned PR merged already for the said review.

No worries! Good idea - I've switched things around so we now get a rendered preview here: https://arai-a.github.io/ecma262-compare/?pr=2777

acutmore avatar May 18 '22 12:05 acutmore

Updated review (looking at the 262 PR)

ljharb avatar May 31 '22 18:05 ljharb

lgtm for stage 3

brad4d avatar May 31 '22 20:05 brad4d

Hi @gibson042 & @ljharb I marked you both as reviewed here as Ashley did go through your comments. Let me know if that is not satisfying to you, I can rescind that review.

@mhofman can you let us know if you had a chance to review?

rricard avatar Jun 06 '22 09:06 rricard