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

Fix 8167 (BigInt identity constructor) and 9061 (BigInt bitwise ops) #1721

Merged
merged 8 commits into from Dec 17, 2013

Conversation

Biotronic
Copy link
Contributor

No description provided.

@monarchdodra
Copy link
Collaborator

Fix 8167 (identity constructor) and 9061 (bitwise ops): LGTM.

Bitwise op behavior: This looks good to me, and is in line with the discussion. However, it would be nice for it to also be publicly documented, so as to avoid any future confusion: eg something like "Bitwise operators behave as if the BigInt was implemented as an infinite length 2's complement number".

@monarchdodra
Copy link
Collaborator

Nitpciks: The current implementations with integrals looks correct. However, converting the integral to a BigInt, followed by doing a binary operation of the length of the biggest of both BigInts seems inefficient, especially for operators '|' and '&'.

It's correct, but I think it leaves room for a future improvement.

Anyways, anybody else want to proof read the math? It looks legit to me, but a second opinion never hurts.

@Biotronic
Copy link
Contributor Author

Yeah, I've also been bothered by the extra allocations and stuff, but with no comments I didn't quite bother. Should be better now.

@monarchdodra
Copy link
Collaborator

Logical operations (|, &, ^, ~) are supported

I thought "logical" was ||, &&, !, ==, < etc? Aren't those "binary" operations?


I searched wikipedia, and according to them, the correct terminology would be: "Bitwise operation" (|, &, ^, ~) and "Boolean expression" (||, &&, !, ==, <, ...).

Other than this final nitpick, I'm ready to merge.

@Biotronic
Copy link
Contributor Author

I used 'logical' because that's what the original said, and I'm willing to argue both names are correct, but I prefer the name 'bitwise' myself. :p

monarchdodra added a commit that referenced this pull request Dec 17, 2013
Fix 8167 (BigInt identity constructor) and 9061 (BigInt bitwise ops)
@monarchdodra monarchdodra merged commit 05738e1 into dlang:master Dec 17, 2013
@monarchdodra
Copy link
Collaborator

And merged! TYVM.

@Biotronic
Copy link
Contributor Author

Wheee!!!

... sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants