Merge in binary fields from libiop, make API consistent between fields, add tests, etc

[alexander-zw]

Note: some file directory structures were changed, which might affect code that uses this as a dependency.

[alexander-zw]

Also changed all tests to use Google test (gtest). It looks like I copied the whole gtest repo. It might have been better as a git submodule.

[AntoineRondelet]

Also changed all tests to use Google test (gtest). It looks like I copied the whole gtest repo. It might have been better as a git submodule.

It seems that the entire gtest repo was added indeed. I think it would be great to remove it to keep the git history strictly "libff-related" and also to make it easier to navigate the diffs of this PR.

You can either remove it via an additional commit, or use things like git filter-branch (see: https://git-scm.com/docs/git-filter-branch) to remove these files from the branch.

[AntoineRondelet]

Aren't the line above tautological? See: #38

[AntoineRondelet]

Oh! I think I understand what @ValarDragon said on my PR. The one == one is a way to test the operator==, is that what you meant @ValarDragon?
I was expecting a separate test for that... i.e. having a test per function (with positive and negative test cases) instead of a test function (like this test_group) testing a bunch of functions at the same time.

[ValarDragon]

Yeah that is what I meant! Sorry I didn't reply to that

[AntoineRondelet]

No problem, thanks for confirming! I get your point, the asserts I removed in #38 are definitely not "dummy/tautological" asserts. They are test cases, so we need to keep them.

That being said, I find the current test structure quite confusing. There is a test_mixed_add() function to test the mixed addition, a test_mul_by_q() function to test mult by field characteristic, a test_output() function for the operator>> and operator<<, and the rest of the functions of the "group interface" are tested in this test_group() function. I find this quite inconsistent - i.e. some functions have their own tests (1 function - 1 associated test), some don't. Additionally, I am not sure that all functions from the group are actually tested here (I can't see any test for is_well_formed for instance).

I personally tend to prefer over verbosity in tests as a way to make it trivial to see what is tested and what is not, and avoid mistakes.

If sticking to a single test test_group is desired here, I think doing something like:

template<typename GroupT>
void test_group{
    test_add<GroupT>(); // test add() function
    test_is_well_formed<GroupT>(); // test is_well_formed()
    test_dbl<GroupT>(); // test dbl()
    test_equal<GroupT>(); // test operator==() (can also test operator!=() or do it in another fucntion)
    ...
    test_is_zero<GroupT>();
    test_mul_by_q<GroupT>();
}

Would be a good improvement, as we would test each function independently, and these tests would be self contained. This would avoid mixing all test cases in one big test function.

Any thoughts?

[alexander-zw]

That makes sense, though I never touched this code other than using gtest. Since this seems to require a relatively large refactor, maybe save it for a future PR.

[AntoineRondelet]

I have opened a ticket for that, see: #46

[ValarDragon]

This seems like a great change for readability to me as well

[AntoineRondelet]

Additionally, it seems that Travis was not ran.
I think @dtebbs opened a PR related to that: #39, maybe worth fixing Travis before proceeding and merging this PR.

[dtebbs]

There seem to be a lot of files committed that are output from the build (binary files or files with cached settings which are machine specific). Primarily in depends/gtest, but possibly elsewhere.

[Pratyush]

I recommend switching to github actions, Travis acts up a lot and is much slower.

[alexander-zw]

I did a cherry pick to change gtest to a git submodule

[AntoineRondelet]

I recommend switching to github actions, Travis acts up a lot and is much slower.

@Pratyush
+1 for Github Action. If I remember correctly, it still doesn't support the ability to re-run a single job (out of many), which is a little bit of a pain when testing a code base on different platforms via multiple jobs etc. Other than that, I think it's pretty good, and would make sense to switch to it for this library.

[dtebbs]

There seems to be rather a lot happening in this PR (and discussion). We might benefit from splitting into multiple PRs. One possible division would be:

• Switch to gtest
• (Switch to GitHub actions? - seems out of scope for this PR)
• Update interface
• Add new fields
  Something like this might make each part easier to evaluate.

[ValarDragon]

I suggest the following structure for new , smaller/easier to review PR's to make these changes, and decouple things.

Suggested sequence of PR's to staging:

First have 1 PR to bring in the new field.hpp interface, which we can comment on, etc.

Once that is merged, then the two following PR's (perhaps in parallel or sequence, whatever you prefer):

• update all existing fields to the new field.hpp
• Bring in libiop binary fields

At any point, have the PR to switch to gtest. I don't know at what point that transition is easiest. (Also we could do a partial transition at first)

@alexander-zw Let me know if you have time to split up the PR, or if you'd like help

[alexander-zw]

All changes in this PR were added in other PRs