Merge pull request #178 from fingolfin/mh/tweaks

simplify julia_to_gap, add test for recursive record

LibGAP.jl/src/ccalls.jl

@@ -112,13 +112,12 @@ Globals = GlobalsType(Dict{Symbol,Cuint}())
 
 function getproperty(funcobj::GlobalsType, name::Symbol)
     cache = getfield(funcobj,:funcs)
-    if haskey(cache, name)
-        gvar = cache[name]
-    else
+
+    gvar = get!(cache, name) do
         name_string = string(name)
-        gvar = ccall(:GVarName, Cuint, (Ptr{UInt8},), name_string)
-        cache[name] = gvar
+        ccall(:GVarName, Cuint, (Ptr{UInt8},), name_string)
     end
+
     v = ccall(:ValGVar, Ptr{Cvoid}, (Cuint,), gvar)
     v = GET_FROM_GAP(v)
     if v == nothing

LibGAP.jl/src/julia_to_gap.jl

@@ -62,59 +62,70 @@ julia_to_gap(x::Symbol) = MakeString(string(x))
 
 ## Generic caller for optional arguments
 julia_to_gap(obj::Any, recursive, recursion_dict ) = julia_to_gap(obj)
-
-## Arrays
-function julia_to_gap(obj::Array{T,1}, recursive::Val{Recursive}=Val(false), recursion_dict = IdDict()) where Recursive where T
+#=
+function julia_to_gap(obj::Any, recursive, recursion_dict )
     if haskey(recursion_dict,obj)
         return recursion_dict[obj]
     end
+    return julia_to_gap(obj)
+end
+=#
+
+## Arrays
+function julia_to_gap(obj::Array{T,1}, recursive::Val{Recursive}=Val(false), recursion_dict = IdDict()) where Recursive where T
     len = length(obj)
     ret_val = NewPlist(len)
     recursion_dict[obj] = ret_val
     for i in 1:len
-        current_obj = obj[i]
-        if haskey(recursion_dict,current_obj)
-            ret_val[i] = recursion_dict[current_obj]
-        else
-            if Recursive
-                current_converted = julia_to_gap(current_obj,recursive,recursion_dict)
+        x = obj[i]
+        if Recursive
+#=
+            # It would be much nicer to write this, but we cannot in Julia 1.x, see
+            # https://github.com/JuliaLang/julia/issues/30165
+            x = get!(recursion_dict, x) do
+                julia_to_gap(x, recursive, recursion_dict)
+            end
+=#
+            if haskey(recursion_dict,x)
+                x = recursion_dict[x]
             else
-                current_converted = current_obj
+                x = recursion_dict[x] = julia_to_gap(x, recursive, recursion_dict)
             end
-            recursion_dict[current_obj] = current_converted
-            ret_val[i] = current_converted
         end
+        ret_val[i] = x
     end
     return ret_val
 end
 
 ## Tuples
 function julia_to_gap(obj::Tuple, recursive::Val{Recursive}=Val(false), recursion_dict = IdDict()) where Recursive
-    size = length(obj)
-    array = Array{Any,1}(undef,size)
-    for i in 1:size
-        array[i] = obj[i]
-    end
+    array = collect(Any, obj)
     return julia_to_gap(array, recursive, recursion_dict)
 end
 
 ## Dictionaries
 function julia_to_gap(obj::Dict{T,S}, recursive::Val{Recursive}=Val(false), recursion_dict = IdDict()) where Recursive where S where T <: Union{Symbol,AbstractString}
-    if haskey(recursion_dict,obj)
-        return recursion_dict[obj]
-    end
-    nr_entries = obj.count
-    keys = Array{T,1}(undef,nr_entries)
-    entries = Array{S,1}(undef,nr_entries)
-    i = 1
+
+    recursion_dict[obj] = record = EvalString("rec()")
     for (x,y) in obj
-        keys[i] = x
-        entries[i] = y
-        i += 1
+        x = GAP.Globals.RNamObj(MakeString(string(x)))
+        if Recursive
+#=
+            # It would be much nicer to write this, but we cannot in Julia 1.x, see
+            # https://github.com/JuliaLang/julia/issues/30165
+            y = get!(recursion_dict, y) do
+                julia_to_gap(y, recursive, recursion_dict)
+            end
+=#
+            if haskey(recursion_dict,y)
+                y = recursion_dict[y]
+            else
+                y = recursion_dict[y] = julia_to_gap(y, recursive, recursion_dict)
+            end
+        end
+        GAP.Globals.ASS_REC(record, x, y)
     end
-    record = GAP.Globals.CreateRecFromKeyValuePairList(julia_to_gap(keys,Val(true)),julia_to_gap(entries, recursive, recursion_dict))
-    ## FIXME: This is too late!
-    recursion_dict[obj] = record
+
     return record
 end
 

LibGAP.jl/test/conversion.jl

@@ -93,13 +93,21 @@
     x = GAP.EvalString("(1,2,3)")
     @test GAP.gap_to_julia(x) == x
 
-    ## Recursive conversions
+    ## Conversions involving circular references
     xx = GAP.EvalString("l:=[1];x:=[l,l];")
     conv = GAP.gap_to_julia(xx)
     @test conv[1] === conv[2]
     conv = GAP.gap_to_julia(Tuple{Tuple{Int64},Tuple{Int64}},xx)
     @test conv[1] === conv[2]
 
+    xx = GAP.EvalString("[~]");
+    conv = GAP.gap_to_julia(xx)
+    @test conv === conv[1]
+
+    xx = GAP.EvalString("rec(a := 1, b := ~)");
+    conv = GAP.gap_to_julia(xx)
+    @test conv === conv[:b]
+
     ## Catch conversions to types that are not supported
     xx = GAP.julia_to_gap( "a" )
     @test_throws MethodError GAP.gap_to_julia( Dict{Int64,Int64}, xx )
@@ -167,19 +175,42 @@ end
 
     ## Dictionaries
     x = GAP.EvalString(" rec( foo := 1, bar := \"foo\" )" )
+    # ... recursive conversion
     y = Dict{Symbol,Any}( :foo => 1, :bar => "foo" )
     @test GAP.julia_to_gap(y,Val(true)) == x
+    # ... non-recursive conversion
     x = GAP.EvalString(" rec( foo := 1, bar := JuliaEvalString(\"\\\"foo\\\"\") )" )
     @test GAP.julia_to_gap(y) == x
 
-    ## Recursive conversions
+    ## Conversions with identical sub-objects
     l = [1]
     yy = [l,l]
-    xx = GAP.EvalString("l:=[1];x:=[l,l];")
-    conv = GAP.julia_to_gap(xx)
-    @test GAP.Globals.IsIdenticalObj(conv[1],conv[2])
+    # ... recursive conversion
+    conv = GAP.julia_to_gap(yy, Val(true))
+    @test isa(conv[1], GAP.MPtr)
+    @test conv[1] === conv[2]
+    # ... non-recursive conversion
+    conv = GAP.julia_to_gap(yy, Val(false))
+    @test isa(conv[1], Array{Int64,1})
+    @test conv[1] === conv[2]
 
-    xx = GAP.EvalString("[~]");
-    @test xx === xx[1]
+    ## converting a list with circular refs
+    yy = Array{Any,1}(undef,2)
+    yy[1] = yy;
+    yy[2] = yy;
+    # ... recursive conversion
+    conv = GAP.julia_to_gap(yy, Val(true));
+    @test conv[1] === conv
+    @test conv[1] === conv[2]
+    # ... non-recursive conversion
+    conv = GAP.julia_to_gap(yy, Val(false))
+    @test conv[1] !== conv
+    @test conv[1] === conv[2]
+
+    ## converting a dictionary with circular refs
+    d = Dict{String,Any}("a" => 1)
+    d["b"] = d
+    conv = GAP.julia_to_gap(d, Val(true));
+    @test conv === conv.b
 
 end