Scalar operations across all integer types #313

feadoor · 2017-06-29T21:28:54Z

With my apologies for opening a new PR, and also for the 8 month delay, this continues the work started in #237 - the discussion there outlines the goals I was aiming for. I suppose this supersedes that PR and the other one can now be closed.

This PR adds support for Add, Sub, Mul, Div and Rem operations involving one BigInt/BigUint and one primitive integer, with operands in either order, and any combination of owned/borrowed arguments.

BigUint and BigInt can now be multiplied by a BigDigit, re-using the same buffer for the output, thereby reducing allocations and copying.

A BigDigit can be added to a BigUint - this is one of several operations being implemented to allow scalar operations on BigInt and BigUint across the board.

A BigDigit can be subtracted from a BigUint - this is one of several operations being implemented to allow scalar operations on BigInt and BigUint across the board.

A BigUint can be divided by a BigDigit - this is one of several operations being implemented to allow scalar operations on BigInt and BigUint across the board.

Allow the addition to occur with either operand order, and with any combination of owned and borrowed arguments.

Allow the multiplication to occur with either operand order and with any combination of owned and borrowed arguments.

Allow the subtraction to occur with either operand order and with any combination of owned and borrowed arguments.

Allow the division to occur with either operand order and with any combination of owned and borrowed arguments.

- Don't apply attributes to statements (1.12.0) - Don't use checked_abs (1.13.0)

cuviper · 2017-06-30T17:02:20Z

Great! I've been meaning to ping you about this. :)

Before I dig into a full review, I'm having second thoughts about doing this for all integer types, because of this one usability wart: type inference fails for unspecified literals.

error[E0277]: the trait bound `num::BigUint: std::ops::Add<i32>` is not satisfied                          
 --> examples/bigint.rs:6:13                                                                               
  |                                                                                                        
6 |     let x = BigUint::zero() + 42;                                                                      
  |             ^^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Add<i32>` is not implemented for `num::BigUint`  
  |                                                                                                        
  = note: no implementation for `num::BigUint + i32`

(that example file isn't committed here, just something I wrote to try it out.)

When rustc can't infer the exact type for an integer literal, it just chooses i32, even though that's not one of the actual possibilities for BigUint. The options I see for us are:

accept that limitation; users will always have to write a specific unsigned literal type, e.g. 42u32.
go ahead and implement Add<i32> for BigUint, and treat negatives like overflow
- maybe mapping to subtraction where possible, and map Sub<i32> negatives to addition.
step back and just implement u32 for BigUint.
- BigInt can have both u32 and i32, as the inference fallback fits in the latter.

What do you think?

feadoor · 2017-06-30T17:28:37Z

What a funny little error! Of your three options, I prefer the first. The compiler error is very clear about what's going on, and it doesn't seem unreasonable to be expected to add the type suffix in this case.

go ahead and implement Add<i32> for BigUint, and treat negatives like overflow

maybe mapping to subtraction where possible, and map Sub<i32> negatives to addition.

I don't like this option. Either i32 becomes a special case, which is confusing to users, or all signed types get implemented for BigUint, which encourages bad behaviour from users - signed primitives should belong with BigInt, and not with BigUint.

step back and just implement u32 for BigUint.

BigInt can have both u32 and i32, as the inference fallback fits in the latter.

I think it's more irritating / confusing for a user to think "Why can't I do this operation on a u64?" than it is for them to be told by the compiler that they need to specify the type of a literal.

cuviper

This is an impressive PR, thanks!

The few things I found are very minor. Unless you happen to be standing by right now, ready to make changes, I'll just go ahead myself.

cuviper · 2017-07-11T22:30:58Z

bigint/src/bigint.rs

@@ -299,6 +302,24 @@ impl Signed for BigInt {
    }
 }

+// A convenience method for getting the absolute value of an i32 in a u32.
+fn i32_abs_as_u32(a: i32) -> u32 {


This probably should be #[inline].

cuviper · 2017-07-11T22:31:09Z

bigint/src/bigint.rs

+}
+
+// A convenience method for getting the absolute value of an i64 in a u64.
+fn i64_abs_as_u64(a: i64) -> u64 {


Also #[inline].

cuviper · 2017-07-11T23:04:41Z

bigint/src/biguint.rs

+            self.data.push(0);
+        }
+
+        let carry = __add2(&mut self.data, &[other]);


For len() == 0 && other == 0, won't this fail the __add2 length assertion?
(could just return self right away for other == 0, of course)

cuviper · 2017-07-11T23:06:54Z

bigint/src/biguint.rs

+        }
+
+        let (lo, hi) = big_digit::from_doublebigdigit(other);
+        let carry = __add2(&mut self.data, &[lo, hi]);


Similarly here, the __add2 length assertion doesn't care if they're zeros.

cuviper · 2017-07-12T05:16:15Z

bors r+

313: Scalar operations across all integer types r=cuviper With my apologies for opening a new PR, and also for the 8 month delay, this continues the work started in #237 - the discussion there outlines the goals I was aiming for. I suppose this supersedes that PR and the other one can now be closed. This PR adds support for Add, Sub, Mul, Div and Rem operations involving one BigInt/BigUint and one primitive integer, with operands in either order, and any combination of owned/borrowed arguments.

bors · 2017-07-12T05:31:08Z

Build succeeded

continuous-integration/travis-ci/push

feadoor added 20 commits June 28, 2017 14:02

Add scalar multiplication to BigUint, BigInt

e520bda

BigUint and BigInt can now be multiplied by a BigDigit, re-using the same buffer for the output, thereby reducing allocations and copying.

Scalar addition of BigDigit to BigUint

a2a28c6

A BigDigit can be added to a BigUint - this is one of several operations being implemented to allow scalar operations on BigInt and BigUint across the board.

Scalar subtraction of a BigDigit from a BigUint

7b7799e

A BigDigit can be subtracted from a BigUint - this is one of several operations being implemented to allow scalar operations on BigInt and BigUint across the board.

Fix typo in comment in division algorithm

530e2f6

Scalar division of a BigUint by a BigDigit

784d26b

A BigUint can be divided by a BigDigit - this is one of several operations being implemented to allow scalar operations on BigInt and BigUint across the board.

Implement all variants of adding BigDigit to BigUint

e5ed503

Allow the addition to occur with either operand order, and with any combination of owned and borrowed arguments.

All variants of multiplying BigUint by BigDigit

fd2f516

Allow the multiplication to occur with either operand order and with any combination of owned and borrowed arguments.

Distinction for commutative scalar ops

5738141

All variants of subtracting BigDigit from BigUint

51408a9

Allow the subtraction to occur with either operand order and with any combination of owned and borrowed arguments.

All variants of dividing BigUint by BigDigit

d0bfb54

Allow the division to occur with either operand order and with any combination of owned and borrowed arguments.

Remove unnecessary normalization

1e26bdd

Also implement scalar addition for BigInt

80feea2

Add scalar subtraction to BigInt

79448cb

Add scalar multiplication to BigInt

8b1288e

Add scalar division to BigInt

9b0392d

Add operations on i32 to BigInt

94d5706

Scalar operations on integer types up to 32 bits

99873d0

Fix normalization in scalar addition

fd87d87

Add scalar ops for all remaining integer types

2a3cd41

Make new code work on rustc-1.8.0

1fb03ca

- Don't apply attributes to statements (1.12.0) - Don't use checked_abs (1.13.0)

cuviper mentioned this pull request Jun 30, 2017

Add scalar multiplication to BigUint, BigInt #237

Closed

cuviper requested changes Jul 11, 2017

View reviewed changes

cuviper added 4 commits July 11, 2017 17:01

inline i32_abs_as_u32 and i64_abs_as_u64

18cc190

fix endianness of to/from_doublebigdigit calls

18a5bfc

test and fix more scalar add cases

6afac82

Add assert_scalar_op! for DRYer testing

e5434dc

cuviper approved these changes Jul 12, 2017

View reviewed changes

bors bot merged commit e5434dc into rust-num:master Jul 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Scalar operations across all integer types #313

Scalar operations across all integer types #313

Uh oh!

feadoor commented Jun 29, 2017

Uh oh!

cuviper commented Jun 30, 2017

Uh oh!

feadoor commented Jun 30, 2017

Uh oh!

cuviper left a comment

Uh oh!

cuviper Jul 11, 2017

Uh oh!

cuviper Jul 11, 2017

Uh oh!

cuviper Jul 11, 2017

Uh oh!

cuviper Jul 11, 2017

Uh oh!

cuviper commented Jul 12, 2017

Uh oh!

bors bot commented Jul 12, 2017

Uh oh!

Uh oh!

Scalar operations across all integer types #313

Scalar operations across all integer types #313

Uh oh!

Conversation

feadoor commented Jun 29, 2017

Uh oh!

cuviper commented Jun 30, 2017

Uh oh!

feadoor commented Jun 30, 2017

Uh oh!

cuviper left a comment

Choose a reason for hiding this comment

Uh oh!

cuviper Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

cuviper Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

cuviper Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

cuviper Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

cuviper commented Jul 12, 2017

Uh oh!

bors bot commented Jul 12, 2017

Build succeeded

Uh oh!

Uh oh!