Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Review feedback #345

Open
rkirsling opened this issue Aug 10, 2022 · 6 comments
Open

Review feedback #345

rkirsling opened this issue Aug 10, 2022 · 6 comments

Comments

@rkirsling
Copy link
Member

Main points:

  1. I was quite confused by the ToString table until I saw Implicit conversion of record to string throws TypeError #319; if we keep this behavior, we should add a note explaining it.

  2. Shouldn't SameValueNonGeneric be called SameValueGeneric? Seems like a typo.

  3. Tuple [[GetOwnProperty]] and [[Get]] are attempting to directly access [[Sequence]] on a Tuple exotic object instead of going through [[TupleData]].

  4. Is it intentional that Record.fromEntries passes the adder Abstract Closure directly to AddEntriesFromIterable while Tuple.from does CreateBuiltinFunction first?

Nitpicky editorial stuff:

  1. RecordPropertyDefinitionEvaluation could use a ? instead of an explicit ReturnIfAbrupt for PropertyName : AssignmentExpression.

  2. In TupleSequenceAccumulation, the use of SpreadElement seems to align with Array, but there would be less redundancy if we align with Record and use TupleElement : AssignmentExpression | ... AssignmentExpression instead.

  3. Kind of annoying that Tuple's Evaluation comes after TupleSequenceAccumulator instead of before it, though I see that this too is in alignment with Array instead of Record.

@rkirsling rkirsling mentioned this issue Aug 10, 2022
25 tasks
@acutmore
Copy link
Collaborator

Thanks @rkirsling this is great. We'll work through these items.

@acutmore
Copy link
Collaborator

  1. I was quite confused by the ToString table until I saw Implicit conversion of record to string throws TypeError #319; if we keep this behavior, we should add a note explaining it.

Good idea!

  1. Shouldn't SameValueNonGeneric be called SameValueGeneric? Seems like a typo.

This AO is a rename of SamevalueNonNumeric. It was previously nonNumeric because the input is not allowed to be a Number or a BigInt. It is now nonGeneric because the input is even less generic because the parameters can now also not be records or tuples.

  1. Tuple [[GetOwnProperty]] and [[Get]] are attempting to directly access [[Sequence]] on a Tuple exotic object instead of going through [[TupleData]].

Great spot!

  1. Is it intentional that Record.fromEntries passes the adder Abstract Closure directly to AddEntriesFromIterable while Tuple.from does CreateBuiltinFunction first?

I don't think so, this looks like a spec bug. Thanks!

@ljharb
Copy link
Member

ljharb commented Aug 10, 2022

I haven't complained about "NonGeneric" yet because I don't have a better suggestion, but my review will hopefully include some :-)

@rkirsling
Copy link
Member Author

rkirsling commented Aug 10, 2022

This AO is a rename of SamevalueNonNumeric. It was previously nonNumeric because the input is not allowed to be a Number or a BigInt. It is now nonGeneric because the input is even less generic because the parameters can now also not be records or tuples.

Yeah, that's why I was viewing it as a typo. The generic (i.e. default) path used to be describable as "non-numeric" because the non-generic (i.e. special) paths were specifically numeric, but with R&T we now need to be more abstract than that.

@acutmore
Copy link
Collaborator

I think I understand the different perspectives now. I am seeing the word generic as "not specific - can handle any input type". So "nonGeneric" seems correct because the AO is specific, it only handles a subset of types. I can also see "Generic" as being interpreted as "handles a related a class of things" similar to WeakSet<T extends object>, where now the AO is 'generic' with a specifier on the group it is generic on.

What do we think a better name could be? Maybe SameValueBase or SameValueCore because it doesn't call out to any other AOs? Or SameValueNonNumericandNonComposite or SVNNNC for short.

@rkirsling
Copy link
Member Author

rkirsling commented Aug 15, 2022

I would say SameValueDefault or SameValueInner, since it's exclusively used after ruling out all the more specific options.
( Base or Core would certainly be in that direction too; I can't seem to find a precedent in scanning through AOs...)

I think the difficulty lies in trying to name it as if it were an operation with a self-standing reason to exist; from that vantage point, I think the best you could say is SameValueTriviallyComparable.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

4 participants