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: Replace the operator tables with explicit steps #3002

Closed
wants to merge 4 commits into from

Conversation

gibson042
Copy link
Contributor

The tables in Assignment Operators Evaluation and ApplyStringOrNumericBinaryOperator are somewhat redundant with each other, and the latter in particular is more complex than #2944 (comment) suggests to me is desirable (especially considering the resulting _operation_(_lnum_, _rnum_) indirection), and incidentally are also the only examples of block-level elements inside an <emu-alg> step. I think they can be clarified and simplified by replacement with explicit algorithm steps.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

i like this

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM

@jmdyck
Copy link
Collaborator

jmdyck commented Jan 26, 2023

Note that this reverses some of the directions of PR #1961:

See also PR #2945, which concluded with "the current editors all prefer the table" (in the semantics for |AssignmentOperator|).

Not objecting, just making historical connections.

@michaelficarra
Copy link
Member

Yeah I'm neutral on the table for compound assignment. But the change to the Number/BigInt operator dispatch is very nice!

@gibson042
Copy link
Contributor Author

gibson042 commented Jan 27, 2023

Note that this reverses some of the directions of PR #1961:

See also PR #2945, which concluded with "the current editors all prefer the table" (in the semantics for |AssignmentOperator|).

Not objecting, just making historical connections.

@jmdyck Much appreciated. This is indeed not even my first crack at this, let alone the first historical attempt. I did link to #2944 (whose rejection prompted the same fate for #2945) in the description, and both of them reference #1961. But even so, I feel like this form of the operations has legs.

Yeah I'm neutral on the table for compound assignment. But the change to the Number/BigInt operator dispatch is very nice!

@michaelficarra Thank you. The compound assignment table does have a simpler single-lookup-key structure, and is therefore less objectionable—my biggest issue is not its existence (I actually like tables, cf. #2944) but rather its placement inside an <emu-alg> step (cf.tc39/ecmarkup#513 (comment) ). If consensus is still to prefer it over text manipulation, then I would propose promoting it into a full <emu-table> at e.g. Assignment Operators or a new operation. Or yet another possibility would be introducing a new syntax-directed operation for |AssignmentOperator|. But the bottom line for me is that having it inside an algorithm step is unnecessarily awkward.

@bakkot bakkot added the editor call to be discussed in the next editor call label Feb 8, 2023
@bakkot
Copy link
Contributor

bakkot commented Feb 8, 2023

@gibson042 we talked about this at the editor call today and couldn't come to an agreement. Personally I think the inline tables are the clearest option of those discussed, so I prefer the status quo. I am slightly negative on the algorithm steps which do string manipulation (since I think they require more thought to follow, plus the assert), and strongly negative on the algorithm steps which determine which AO to call (since I think they obscure what's going on, particularly since four of the 24 are different from the rest). But @michaelficarra was very positive about the second change.

We'll talk about it a bit more next call, but unless we come to agreement we'll probably end up sticking with the status quo.

@bakkot
Copy link
Contributor

bakkot commented Feb 15, 2023

We talked about this again and are happier with the status quo. We did discuss the option of factoring out the tables into their own steps, or introducing SDOs to do the parsing, but those both seem less readable. Thanks for the suggestion, though.

@bakkot bakkot closed this Feb 15, 2023
@bakkot bakkot removed the editor call to be discussed in the next editor call label Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants