finite_field: Define trait FieldElement and add more fields

[cjpatton]

Partially addresses #10. This PR makes the following changes:

• Defines a new trait, FieldElement, that specifies the API currently implemented by Field, as well as some additional features.
• Adds a new macro, make_field!(), which wraps a FieldParameters into an implementation of FieldElement.
• Changes the implementation of the 32-bit field Field so that it is constructed using make_field!().
• Adds three additional fields: Field64, Field80, and Field126.

Thanks to @tgeoghegan for the tip about super traits.

[tgeoghegan]

I think we could do "better" (for some definition of better that I refuse to provide) if we had the const generics feature that just made beta in Rust 1.51, and then FieldElement could be generic over values of FieldParameters. However I don't want to require a beta or nightly compiler, and the macro solution here is more than good enough given the small number of FieldParameter values we support. I filed #18 so we can revisit const generics once they are in stable rustc. Otherwise I just have some nits about values and references and more impls, but this looks great to me!

[tgeoghegan]

I like the use of an associated type!

[tgeoghegan]

Suggested change

	fn pow(self, exp: Self) -> Self;
	fn pow(&self, exp: Self) -> Self;

As written, this will move the value self into the method pow, making it illegal to reference the value later. Here's a playground illustrating the problem.

This is true because FieldElement does not require std::marker::Copy, and so its values have move semantics. However, all implementations of FieldElement stamped out by make_field! do implement Copy (which is reasonable, since they just wrap a u128, and thus a bitwise copy is pretty cheap and semantically valid), so if you did want to make pow take a value (for instance, it might spare callers a & in some contexts), you could just extend FieldElement to also be a supertrait over Copy.

My vote would be to take a reference, to avoid unnecessarily restricting FieldElement implementations with Copy, and because taking an immutable reference is useful as a shorthand way to communicate to callers that the method won't mutate, destroy or consume the receiver.

[cjpatton]

Great catch! In fact, this came up in a follow-up PR I'm working on, but there I didn't really think through what the solution ought to be. I ended up following the compiler and make Copy a supertrait of FieldElement. Your solution is better.

Done. I applied a similar to change to inv() below.

[tgeoghegan]

I was a bit surprised to learn that std::ops::Add::add takes self as a value, since Add doesn't require that the receiver implement std::marker::Copy. I came across this in the std::ops documentation:

Many of the operators take their operands by value. In non-generic contexts involving built-in types, this is usually not a problem. However, using these operators in generic code, requires some attention if values have to be reused as opposed to letting the operators consume them. One option is to occasionally use clone. Another option is to rely on the types involved providing additional operator implementations for references. For example, for a user-defined type T which is supposed to support addition, it is probably a good idea to have both T and &T implement the traits Add and Add<&T> so that generic code can be written without unnecessary cloning.

WDYT about also providing

        impl std::ops::Add for &$elem {
            type Output = $elem;
            fn add(self, rhs: Self) -> $elem {
                $elem($fp.add(self.0, rhs.0))
            }
        }

?

And same goes for all the other std::ops trait implementations

[cjpatton]

1. I like this idea, but I would prefer re-using the implementation of Add for $elem:

         impl Add for &$elem {
             type Output = $elem;
             fn add(self, rhs: Self) -> $elem {
                 *self + *rhs
             }
         }

2. What's the analogous pattern for the assignment operators, e.g., AddAssign for &$elem? Does this even make sense?

[tgeoghegan]

1. I agree, yours is better code reuse.
2. std::ops::AddAssign::add_assign explicitly takes &mut self and returns nothing (which makes sense, the point of += is to mutate the value) so I don't think it makes sense to replicate the pattern there. I think the extra impls are only necessary/helpful for those traits in std::ops where the method receiver is a value.

[cjpatton]

Done, for Add, Sub, Mul, and Div, and Neg.

[tgeoghegan]

LGTM, with one nit. Could you please write some issues corresponding to the TODO comments?

[tgeoghegan]

Actually, wait: I notice now that the build produces some warnings.

warning: associated function is never used: `rand_elem`
   --> src/fp.rs:224:12
    |
224 |     pub fn rand_elem<R: Rng + ?Sized>(&self, rng: &mut R) -> u128 {
    |            ^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: associated function is never used: `size`
   --> src/fp.rs:240:12
    |
240 |     pub fn size(&self) -> usize {
    |            ^^^^

warning: 2 warnings emitted

Can these be silenced?

[cjpatton]

• Added a comment to Add support for more fields #10 regarding the open TODO(cjpatton)'s.
• Resolved the compiler warnings by adding the #[cfg(test)] attribute to the functions.

[tgeoghegan]

LGTM!

[dconnolly]

FYI Rust's const generics just landed in stable 🥳

[cjpatton]

NOW @dconnolly tells us!!