Add tests for GMP code and start overhauling it #1045
Conversation
fingolfin
added
the
do not merge
Current coverage is 50.56% (diff: 83.95%)
|
fingolfin
force-pushed
the
mh/gmp
branch
8 times, most recently
from
383d92a
to
24ee4b1
Compare
|
Note to potential reviewer: I tried to make each commit self contained, and for a thorough review, reviewing each commit on its own is probably easiest. |
fingolfin
force-pushed
the
mh/gmp
branch
2 times, most recently
from
cfcd377
to
70da510
Compare
|
I also tried to edit the description of this PR so that it can be reused for a merge commit... |
fingolfin
changed the title
This was added 2010-11-02 as an experimental feature by Laurent. But it was never documented, nor hooked up to anything, or usable without modifying the library.
fingolfin
added
kind: enhancement
|
This PR is now stable (i.e. I don't plan to rewrite its commits anymore, unless changes request are made). Reviews are welcome! |
|
Looking at the [current codecov.io report for gmpints.c]https://codecov.io/gh/gap-system/gap/src/0b11a1541fd3e9cb56456ab575a83272f4b16e04/src/gmpints.c), coverage is now above 95%. It really is even higher, but for some reason, multiple @ChrisJefferson perhaps you have an idea what is happening here? |
There was a problem hiding this comment.
My only concern with this patch is the increased requirement on correctly putting all integers that would fit into a small int, correctly into a small int.
After a quick search I found:
IntHexString("-1000000000000000")
Incorrectly produces a GMPINT which would fit into a small integer (can be fixed with a GMP_REDUCE after line 682).
Looking through gmpints.c, I see a few commented out GMP_REDUCE/GMP_NORMALIZE, for example 1175. I haven't looked carefully to see if this is required, and why it is commented out, but now it would produce incorrect answers if it is wrong.
| @@ -297,6 +318,38 @@ Obj GMP_REDUCE( Obj gmp ) | |||
| return gmp; | |||
| } | |||
|
|
|||
| #if DEBUG_GMP | |||
There was a problem hiding this comment.
I would just always include this function, in case (for example) a package wants to call it, or someone wants to insert it quickly for testing. Also less likely to break it.
I agree with only calling it for DEBUG_GMP of course.
There was a problem hiding this comment.
I am not sure about using IS_NORMALIZED_AND_REDUCED from packages (ideally, packages should never assume that they know how GAP integers work internally; I'd prefer to add APIs that do whatever packages need... but of course that just an idealistic wish, and not realistic right now).
In any case, a strong reason why we should indeed always compile IS_NORMALIZED_AND_REDUCED is to preven bitrot in it. I'll make that change.
|
I thought careful about imposing that requirement on ints always being normalized and reduced. Fact is, a lot of the code already implicitly required that to be correct. In the end, making this the official rule seemed to be the sanest way to cleanup things. Thanks for pointing out the issue with As to the commentend out GMP_REDUCE/GMP_NORMALIZE: these are safe to remove (I had CHECK_GMP calls in there to double check for quite some time). This, too, was not removed on this branch mainly because I completely rewrote the relevant functions. I'll still update it on this branch, too (it's a bit annoying, as it duplicates work, but makes it clearer that the work on this PR is correct). |
|
Updated the PR based on @ChrisJefferson's feedback; I also added |
|
@ChrisJefferson @markuspf @frankluebeck I guess this PR is too big and hence does not get any reviews ... :-). While I am reasonably confident that it is good-to-go as it is, perhaps it would help if I split it into a series of more manageble PRs? Alternatively, I point out once again that one can review this commit by commit... (Or you can not review it and just say "guess it's fine, we trust you to clean up any fall out it may cause" ;-) |
* NEW_INTNEG was commented out and also trivial to reimplement * ProdIntObjFunc and PowObjIntFunc were declared but never used * SMALLEST_INTPOS was exported to the language level, but unused
This change should be mostly irrelevant.; indeed, the C99 standard mandates that a%b and a%(-b) are equal, and even on most pre-C99 compiler this holds true -- but not all. We could instead also add a configure test, and reject compilers violating this; in that case, we can obviously just stop checking the sign of k.
In particular, if INTEGER_UNIT_SIZE ever differs from 2, 4, 8, produce an error. Also replace some tabs by spaces.
The result should also be a bit faster when comparing a small with a large integer, as we immediately return false, instead of first converting the small integer into a large one, then calling mpn_cmp. This relies on all integers always being normalized and reduced.
We cannot simply use ABS_INT and SIGN_INT, as one might expect given the names AbsInt/SignInt, as a lot of library code (and likely in packages and elsewhere) is relying on the undocumented fact that the old AbsInt and SignInt implementations also worked for rationals.
AbsoluteValue used to invoke AbsInt on non-integer rationals. While this works, this is undocumented
This was added 2010-11-02 as an experimental feature by Laurent. But it was never documented, nor hooked up to anything, or usable without modifying the library.
This allows us to optimize it.
Before returning the computed result, ModInt checked whether it was negative, it flipped the sign. This should (a) never be necessary, because the preceding computations should always produce something non-negative, and (b) would be mathematically incorrect. Instead, we simply return the result untreated, and added an assertion triggering if we ever see a negative result here (only enabled if DEBUG_GMP is set).
To turn this on, #define DEBUG_GMP 1
This fix a bug, as IntHexString could return a non-reduced integer.
|
So, merge this? Rebase it? Should I do it myself? |
|
No-one else has any opinions, so I'll do it. |
This PR does a couple of things with the GMP integer code in GAP. I
decided to split it into two parts. This is the first, and hopefully
uncontrovesial part, which lays the foundation for future improvements.
Some highlights:
src/gmpints.c,giving confidence in the correctness of subsequent changes
instead of an error
LtIntandEqInteven got faster in some caseschecking whether a number is even / odd)
previously (needlessly) visible to the outside; this will help with
future improvements
AbsInt and SignInt
from the GAP library