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: make NumberBitwiseOp, BigIntBitwiseOp, ByteListBitwiseOp consistent #1998

Merged
merged 1 commit into from
May 20, 2020

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented May 19, 2020

Ensure op is a String in all cases, and ensure all three operations assert on the contents of op.

Previously, NumberBitwiseOp and ByteListBitwiseOp did not assert on op, and they were passing an operator as if it was a first-class value. Instead, I made all three use the approach of BigIntBitwiseOp.

An alternative to strings would be to use ~^~ etc; happy to switch them all to that form if that's preferred.

@ljharb ljharb requested review from syg, michaelficarra, bakkot and a team May 19, 2020 06:48
@michaelficarra
Copy link
Member

I prefer the token to the string. It feels like a bit less indirection to me.

@ljharb
Copy link
Member Author

ljharb commented May 19, 2020

Like ~|~ etc?

@michaelficarra
Copy link
Member

Yeah.

@ljharb ljharb force-pushed the bitwise_consistency branch from bc2a813 to e530532 Compare May 19, 2020 16:21
@ljharb
Copy link
Member Author

ljharb commented May 19, 2020

@michaelficarra updated!

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@ljharb ljharb force-pushed the bitwise_consistency branch from e530532 to 8dfcc0f Compare May 19, 2020 17:47
@ljharb ljharb requested a review from jmdyck May 19, 2020 17:47
jmdyck
jmdyck previously requested changes May 19, 2020
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@ljharb ljharb force-pushed the bitwise_consistency branch from 8dfcc0f to 732a115 Compare May 20, 2020 03:57
@ljharb ljharb requested a review from jmdyck May 20, 2020 03:57
@michaelficarra
Copy link
Member

@ljharb In 12.15.6 (Runtime Semantics: EvaluateStringOrNumericBinaryExpression), we describe the opText parameter as a sequence of Unicode code points. Do we need to change that as well?

I also don't like how (as far as I can tell) we don't explicitly say anywhere that, for example, ^ means the bitwise xor operation. In NumberBitwiseOp, we just say

Return the result of applying the bitwise operator op to lnum and rnum.

Do we actually say anywhere that ^ means xor or are we just supposed to infer that from the names of productions and AOs?

@ljharb
Copy link
Member Author

ljharb commented May 20, 2020

I believe we’re just supposed to infer it, which I’m not happy with either.

I’ll update 12.15.6 for sure; in the meantime, I’d either be happy to take a suggestion for how to specify this stuff better in this PR, or also to do it in a followup so that this one can at least remove a bunch of inconsistency.

@ljharb
Copy link
Member Author

ljharb commented May 20, 2020

@michaelficarra hmm, i'm actually not sure how to update 12.15.16 without solving that larger problem. Would you be OK with flipping this PR back to using strings, and then in a followup, we can tackle the larger issue of how these are defined and specified?

@michaelficarra
Copy link
Member

michaelficarra commented May 20, 2020

@ljharb Yeah, let's go back to strings for now. I'll open an issue to track being more explicit with numeric operations.

edit: See #2002.

@ljharb ljharb force-pushed the bitwise_consistency branch from 732a115 to 6cece8f Compare May 20, 2020 21:34
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@ljharb ljharb force-pushed the bitwise_consistency branch from 6cece8f to 267e674 Compare May 20, 2020 22:11
@ljharb ljharb force-pushed the bitwise_consistency branch from 267e674 to 9fa1760 Compare May 20, 2020 22:35
@ljharb ljharb dismissed jmdyck’s stale review May 20, 2020 22:35

requested changes made

…eOp` consistent

Ensure `op` is a token in all cases, and ensure all three operations assert on the contents of `op`.

Fixes tc39#1989.
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!

<h1>ByteListBitwiseOp( _op_, _xBytes_, _yBytes_ )</h1>
<p>The abstract operation ByteListBitwiseOp takes arguments _op_ (a read-modify-write modification function), _xBytes_ (a List of byte values), and _yBytes_ (a List of byte values). The operation atomically performs a bitwise operation on all byte values of the arguments and returns a List of byte values. It performs the following steps when called:</p>
<h1>ByteListBitwiseOp ( _op_, _xBytes_, _yBytes_ )</h1>
<p>The abstract operation ByteListBitwiseOp takes arguments _op_ (a sequence of Unicode code points), _xBytes_ (a List of byte values), and _yBytes_ (a List of byte values). The operation atomically performs a bitwise operation on all byte values of the arguments and returns a List of byte values. It performs the following steps when called:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, good catch and thanks for fixing.

@ljharb ljharb merged commit 9fa1760 into tc39:master May 20, 2020
@ljharb ljharb deleted the bitwise_consistency branch May 20, 2020 22:50
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