two's-complement logic operations on BigInt #26

tspiteri · 2018-02-27T02:55:48Z

This commit adds the following impls, giving results equivalent to what would be expected with two's complement.

impl Not for BigInt
impl BitAnd<&BigInt> for BigInt, impl BitAndAssign<&BigInt> for BigInt
impl BitOr<&BigInt> for BigInt, impl BitOrAssign<&BigInt> for BigInt
impl BitXor<&BigInt> for BigInt, impl BitXorAssign<&BigInt> for BigInt

I also added some tests. There is most probably some room for optimization, but the bitwise operations all operate in place using a single pass.

I did not add other implementations such as impl BitAnd<BigInt> for BigInt, those can be done later.

Edit: Fixes #13

cuviper

I just moved the tests around in #27, which causes a conflict here. I suggest making a standalone tests/bigint_bitwise.rs for your new tests.

I haven't looked too closely at the implementation details, but it seems pretty thorough AFAICT. Getting the tests absolutely right helps confidence here. Maybe some randomized i64 inputs would be a good idea for comparison too, in case there are edge cases you didn't think about.

cuviper · 2018-02-27T06:40:46Z

src/tests/bigint.rs

+    N,
+    P(&'static [BigDigit]),
+    M(&'static [BigDigit]),
+}


I suggest use V::*; for readability, to get rid of the V:: prefix everywhere else.

cuviper · 2018-02-27T06:44:23Z

src/tests/bigint.rs

+        assert_eq!(a.clone() | &b, or, "{:x} | {:x}", a, b);
+        assert_eq!(b.clone() | &a, or, "{:x} | {:x}", b, a);
+        assert_eq!(a.clone() ^ &b, xor, "{:x} ^ {:x}", a, b);
+        assert_eq!(b.clone() ^ &a, xor, "{:x} ^ {:x}", b, a);


It would be nice to also have these tests compared to the primitive operators, probably just with i64, at least for those where the values do fit. Just as a sanity check that your test setup is correct.

cuviper · 2018-02-27T06:45:15Z

src/tests/bigint.rs

+        let a = BigInt::from(a);
+        let not = BigInt::from(not);
+        assert_eq!(!a.clone(), not, "!{:x}", a);
+        assert_eq!(!not.clone(), a, "!{:x}", not);


Here too, I think comparing to i64 is worthwhile.

cuviper · 2018-02-27T06:49:10Z

src/bigint.rs

+    fn not(mut self) -> BigInt {
+        match self.sign {
+            Sign::NoSign => {
+                biguint::digits_mut(&mut self.data).push(1);


We support scalar arithmetic -- why not just self.data += 1?

cuviper · 2018-02-27T06:51:06Z

src/bigint.rs

+                self.sign = Sign::Minus;
+            }
+            Sign::Minus => {
+                sub2(biguint::digits_mut(&mut self.data), &[1]);


self.data -= 1?

cuviper · 2018-02-27T06:51:29Z

src/bigint.rs

+                let carry = __add2(biguint::digits_mut(&mut self.data), &[1]);
+                if carry != 0 {
+                    biguint::digits_mut(&mut self.data).push(carry);
+                }


self.data += 1?

cuviper · 2018-02-27T07:08:23Z

src/bigint.rs

+
+// -1 ^ +ff = ...ffff ^ ...00ff = ...ff00 = +100
+// -ff ^ +1 = ...ff01 ^ ...0001 = ...ff00 = +100
+// answer is pos, has length of longest with a possible carry


answer is neg, but I think you got that right in practice.

tspiteri · 2018-02-27T23:52:02Z

I addressed your points in the first commit.

In the second commit I added tests on i64 values within ±3 of interesting values (i64::MIN, -u32::MAX, etc.). All logic operations are tested on all combinations with both i64 and BigInt, and the answers compared. That found a couple of bugs I fixed. I also added a few debug_assertions.

tspiteri · 2018-02-28T15:05:26Z

I've added a third commit which makes the inline docs more helpful. I also made the use of the debug_assertions more consistent. I think this makes the code easier to understand.

cuviper

Some minor things can be simplified, then I'm happy. I'm impressed by how well the single abstraction of negate_carry makes this viable!

Let's not forget the val/ref combinations too, but that can be done later, as you say.

cuviper · 2018-03-04T00:42:55Z

src/bigint.rs

+                self.assign_from_slice(Sign::NoSign, &[]);
+                return;
+            }
+            (Sign::Plus, Sign::Plus) => {


Can't this just use self.data &= other.data?

cuviper · 2018-03-04T00:43:45Z

src/bigint.rs

+                self.assign_from_slice(other.sign, biguint::digits(&other.data));
+                return;
+            }
+            (Sign::Plus, Sign::Plus) => {


Could be self.data |= other.data.

cuviper · 2018-03-04T00:45:04Z

src/bigint.rs

+                self.assign_from_slice(other.sign, biguint::digits(&other.data));
+                return;
+            }
+            (Sign::Plus, Sign::Plus) => {


Could be self.data ^= other.data.

cuviper · 2018-03-04T00:48:14Z

src/biguint.rs

@@ -1686,6 +1686,24 @@ pub fn trailing_zeros(u: &BigUint) -> Option<usize> {
        .map(|(i, digit)| i * big_digit::BITS + digit.trailing_zeros() as usize)
 }

+// needed since we cannot set visibility of `BigUint::data` to `pub(crate)`


I think we should just move struct BigUint to the crate root, and maybe its impl { fn normalize() } too.

I'm not sure I understand this point. I have done something in the new commit 3 (crate-visible IntDigits) that maybe addresses this, but I'm not sure if that's what you had in mind. Anyway, I think further changes here can be independent of this PR.

I meant that if we moved the struct definition to the crate root, then everything could just access the data field directly. The digits_mut function especially was defeating any pretense of privacy within the crate anyway.

But now with your trait, I see how this is helpful to present a common interface for the macros, so this solution seems fine.

cuviper · 2018-03-04T00:52:24Z

src/bigint.rs

+    fn not(mut self) -> BigInt {
+        match self.sign {
+            Sign::NoSign | Sign::Plus => {
+                self.data += 1 as BigDigit;


I guess you got tripped up by this defaulting to 1i32? Let's just use 1u32 instead of casting.

You guessed right :)

tspiteri · 2018-03-04T10:46:30Z

Commit 1 is now the implementation and commit 2 is the tests.

Commit 3 introduces a crate-visible IntDigits trait so that macros that used to use data.len() or data.capacity() can work for both BigUint and BigInt, and I also used it for BigInt to access BigUint::normalize. I made <BigUint as IntDigits>::normalize simply call the inherent method BigUint::normalize, maybe the implementation could move into the trait method and the inherent method can simply be removed?

Commit 4 implements ref/val forwarding for the bitwise ops of BigInt (depends on commit 3).

cuviper · 2018-03-06T06:21:50Z

I took the small liberty of adding reference Not, but otherwise this quite comprehensive. Thanks a lot!

bors r+

26: two's-complement logic operations on BigInt r=cuviper a=tspiteri This commit adds the following impls, giving results equivalent to what would be expected with two's complement. * `impl Not for BigInt` * `impl BitAnd<&BigInt> for BigInt`, `impl BitAndAssign<&BigInt> for BigInt` * `impl BitOr<&BigInt> for BigInt`, `impl BitOrAssign<&BigInt> for BigInt` * `impl BitXor<&BigInt> for BigInt`, `impl BitXorAssign<&BigInt> for BigInt` I also added some tests. There is most probably some room for optimization, but the bitwise operations all operate in place using a single pass. I did not add other implementations such as `impl BitAnd<BigInt> for BigInt`, those can be done later. Edit: Fixes #13

bors · 2018-03-06T06:29:53Z

Build succeeded

continuous-integration/travis-ci/push

cuviper requested changes Feb 27, 2018

View reviewed changes

tspiteri force-pushed the bitwise-bigint branch from 99876ce to 94de40d Compare February 27, 2018 23:49

tspiteri force-pushed the bitwise-bigint branch from 14a0976 to d139cf4 Compare February 28, 2018 15:22

cuviper reviewed Mar 4, 2018

View reviewed changes

tspiteri added 2 commits March 4, 2018 10:21

two's-complement bitwise operations on BigInt

b16359c

add tests for two's-complement bitwise operations

e005dcc

tspiteri force-pushed the bitwise-bigint branch 2 times, most recently from 269ade7 to 57c3a0e Compare March 4, 2018 10:23

tspiteri added 2 commits March 4, 2018 11:34

crate-visible IntDigits trait for common BigUint and BigInt access

c5d6c2c

val/ref forwarding for BigInt bitwise operations

959130e

tspiteri force-pushed the bitwise-bigint branch from 57c3a0e to 959130e Compare March 4, 2018 10:35

impl<'a> Not for &'a BigInt

ec0113e

cuviper approved these changes Mar 6, 2018

View reviewed changes

bors bot merged commit ec0113e into rust-num:master Mar 6, 2018

tspiteri deleted the bitwise-bigint branch March 6, 2018 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

two's-complement logic operations on BigInt #26

two's-complement logic operations on BigInt #26

tspiteri commented Feb 27, 2018 •

edited

Loading

cuviper left a comment

cuviper Feb 27, 2018

cuviper Feb 27, 2018

cuviper Feb 27, 2018

cuviper Feb 27, 2018

cuviper Feb 27, 2018

cuviper Feb 27, 2018

cuviper Feb 27, 2018

tspiteri commented Feb 27, 2018 •

edited

Loading

tspiteri commented Feb 28, 2018

cuviper left a comment

cuviper Mar 4, 2018

cuviper Mar 4, 2018

cuviper Mar 4, 2018

cuviper Mar 4, 2018

tspiteri Mar 4, 2018

cuviper Mar 6, 2018

cuviper Mar 4, 2018

tspiteri Mar 4, 2018

tspiteri commented Mar 4, 2018

cuviper commented Mar 6, 2018

bors bot commented Mar 6, 2018

two's-complement logic operations on BigInt #26

two's-complement logic operations on BigInt #26

Conversation

tspiteri commented Feb 27, 2018 • edited Loading

cuviper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tspiteri commented Feb 27, 2018 • edited Loading

tspiteri commented Feb 28, 2018

cuviper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tspiteri commented Mar 4, 2018

cuviper commented Mar 6, 2018

bors bot commented Mar 6, 2018

Build succeeded

tspiteri commented Feb 27, 2018 •

edited

Loading

tspiteri commented Feb 27, 2018 •

edited

Loading