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

BigInt bitwise and, or, xor #1300

Merged
merged 2 commits into from
Oct 26, 2017
Merged

Conversation

thejoshwolfe
Copy link
Contributor

No description provided.

@rwaldron
Copy link
Contributor

This is not a review "blocker", but I'm wondering what you think about using binary integer literal bigints in these tests? For example:

-assert.sameValue(Object(5n) & 3n, 1n, "Object(5n) & 3n === 1n");
-assert.sameValue(3n & Object(5n), 1n, "3n & Object(5n) === 1n");
+assert.sameValue(Object(0b101n) & 0b011n, 0b001n, "Object(0b101n) & 0b011n === 0b001n");
+assert.sameValue(0b011n & Object(0b101n), 0b001n, "0b011n & Object(0b101n) === 0b001n");

I will understand if that's a bunch of work that you don't want to do ;)

@thejoshwolfe
Copy link
Contributor Author

I like the idea of making the tests more readable. Perhaps using binary literals for the small values? I certainly don't want to write 0xbf2ed51ff75d380fd3be813ec6185780n as a binary literal.

There's not much I can do to make the cases with opposite sign more readable; i still find those pretty hard to understand. 0b11 & -0b11 === 0b01 Writing that with binary literals doesn't really clarify anything. I'd like to leave those cases as they are, since the binary representation might actually be more confusing.

How about I change the cases to use binary notation where 0 <= a <= 7 and 0 <= b <= 7? That would cover all the bigint-non-primitive.js cases, and the first 15 asserts in the bigint.js tests (everything before the first 0xffffffffn case).

@rwaldron
Copy link
Contributor

I certainly don't want to write 0xbf2ed51ff75d380fd3be813ec6185780n as a binary literal.

hahahaha definitely not.

@rwaldron
Copy link
Contributor

How about I change the cases to use binary notation where 0 <= a <= 7 and 0 <= b <= 7? That would cover all the bigint-non-primitive.js cases, and the first 15 asserts in the bigint.js tests (everything before the first 0xffffffffn case).

Sorry, I only meant to suggest using them for bitwise operations :)

@thejoshwolfe
Copy link
Contributor Author

Sorry, I only meant to suggest using them for bitwise operations :)

Sorry, I don't understand. This PR has 6 files; I am suggesting changes in all 6 files. For the 3 files named bigint-non-primitive.js, converting all the BigInt literals to use 0bxxxn format. For the 3 files named bigint.js converting the BigInt literals in the first 15 asserts to use 0bxxn.

@rwaldron
Copy link
Contributor

Sorry, I don't understand. This PR has 6 files; I am suggesting changes in all 6 files.

The misunderstanding was mine, and I apologize. I just misread what you said.

@rwaldron rwaldron merged commit af2e776 into tc39:master Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants