Fix more JET issues #951
Fix more JET issues #951
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #951 +/- ##
==========================================
+ Coverage 75.70% 75.77% +0.07%
==========================================
Files 51 51
Lines 4173 4177 +4
==========================================
+ Hits 3159 3165 +6
+ Misses 1014 1012 -2
|
fingolfin
force-pushed
the
mh/more-JET
branch
from
d68647d to
891091a
Compare
src/julia_to_gap.jl
Outdated
| @@ -219,7 +219,7 @@ function julia_to_gap( | |||
| elseif Wrappers.IsRecord(obj) | |||
| ret_val = NewPrecord(0) | |||
| recursion_dict[obj] = ret_val | |||
| for x in Vector{String}(Wrappers.RecNames(obj)) | |||
| for x in Wrappers.RecNames(obj)::GapObj | |||
There was a problem hiding this comment.
This was a genuine bug here -- we are missing code coverage here.
There was a problem hiding this comment.
Yes, this was wrong.
Looking at the function again, I am also not sure whether the statement
We have to do something only if recursive conversion is required
is correct: Don't we have to make at least a shallow copy?
| @@ -219,7 +219,7 @@ function julia_to_gap( | |||
| elseif Wrappers.IsRecord(obj) | |||
| ret_val = NewPrecord(0) | |||
| recursion_dict[obj] = ret_val | |||
| for x in Vector{String}(Wrappers.RecNames(obj)) | |||
| for x in Wrappers.RecNames(obj)::GapObj | |||
| Wrappers.ASS_REC(ret_val, x, julia_to_gap(Wrappers.ELM_REC(obj, x), recursion_dict; recursive = true)) | |||
There was a problem hiding this comment.
(and this code is still wrong, I'll look into fixing it
There was a problem hiding this comment.
What about
xx = Wrappers.RNamObj(x)
Wrappers.ASS_REC(ret_val, xx, julia_to_gap(Wrappers.ELM_REC(obj, xx), recursion_dict; recursive = true))
There was a problem hiding this comment.
Sounds good. I just updated the PR accordingly and added a test.
| @@ -136,17 +136,17 @@ whereas `x` in the latter example lives in the Julia session. | |||
| """ | |||
| function evalstr(cmd::String) | |||
| res = evalstr_ex(cmd * ";") | |||
| if any(x->x[1] == false, res) | |||
| if any(x::GapObj->x[1] === false, res) | |||
There was a problem hiding this comment.
I read this as the recommendation to use always === or !== when comparing something with true, false, GAP.Globals.fail.
There was a problem hiding this comment.
Yeah. It at least ensures that the compiler can always turn this into the most optimal code; here, it can't predict the type of x[1] so it should check at runtime whether there is a == method for whatever x[1] is plus a Bool... But we have extra information about the intent of the code, so that === is what we want, and can totally away runtime dispatch here.
fingolfin
force-pushed
the
mh/more-JET
branch
from
891091a to
1a0e415
Compare
Some found via @report_opt
fingolfin
force-pushed
the
mh/more-JET
branch
from
1a0e415 to
eb677ba
Compare
Some found via @report_opt