Restructure relations of LibGAP.jl and JuliaInterface #97
Conversation
Codecov Report
@@ Coverage Diff @@
## master #97 +/- ##
==========================================
+ Coverage 51.24% 51.38% +0.13%
==========================================
Files 33 33
Lines 1969 1919 -50
==========================================
- Hits 1009 986 -23
+ Misses 960 933 -27
|
sebasguts
changed the title
|
I am still working on this, no need to review right now. |
sebasguts
force-pushed
the
libgap_restructure
branch
from
99e671b
to
d4713d3
Compare
|
@fingolfin @ThomasBreuer I am finished with this so far, please have a look. @ThomasBreuer I think almost all of the |
There was a problem hiding this comment.
This looks nice.
(Sorry but I do not feel competent to judge about the quality.)
In particular, I will follow the suggestions to replace ccall by direct calls in JuliaExperimental.
Just for curiosity:
You seem to get rid of almost all cases where GET_FROM_GAP is used;
is there a reason to keep it?
|
I am currently not sure. I would guess there will be more ccalls in the future, which might require it, too. I could remove it and just fuse it into the function which it uses, but I see no urgent need to do it, so I left it. |
There was a problem hiding this comment.
All in all this seems fine to me, with some minor remarks. I'd feel better if the new code was tested, but given that it was mostly untested before (I think, at least?), I wouldn't want to hold back this PR over it. Though I do think it would be important to work on these tests (and not just to play "increasing coverage" as a number game for itself, but rather because I am genuinely concerned about bugs in some of that code.
If PR #90 is merged soon, it might be interesting to rebase this PR, just to see how much of this code actually is exercised.
| (Ptr{Cvoid}, Ptr{Cvoid}), a.obj.ptr, int_ptr ) ) | ||
| end | ||
|
|
||
| function ==( a::Int, b::GAPRat ) | ||
| int_ptr = ccall( Main.gap_INTOBJ_INT, Ptr{Cvoid}, (Int,), a ) | ||
| return Bool( ccall( Main.gap_MyFuncEQ, Int, | ||
| int_ptr = ccall( :ObjInt_Int, Ptr{Cvoid}, (Int,), a ) |
There was a problem hiding this comment.
Great, it seems you resolve #70
- Unused functions in gaptypes.jl - Unnecessary setting of JULIA_CPOINTER
sebasguts
dismissed stale reviews from fingolfin and ThomasBreuer
via
6e19ce7
sebasguts
force-pushed
the
libgap_restructure
branch
from
d4713d3
to
6e19ce7
Compare
|
Currently, the tests fail with a strange error. I will investigate. |
| @@ -12,8 +12,6 @@ import Base: zero, -, one, inv, ==, isless, +, *, //, ^, mod, iszero, string, | |||
|
|
|||
| import Main.GAP: GapObj | |||
|
|
|||
| import Main.GAP.GAPFuncs: SUM | |||
|
|
|||
There was a problem hiding this comment.
I had to add this to make it possible to load this file (good thing we fixed the tests :-)
GAP.GAPFuncsto dynamically fetch functions from GAP