JuliaAstro / SPICE.jl

Julia wrapper for NASA NAIF's SPICE toolkit
MIT License
54 stars 14 forks source link

Remove all useless calls to `GC.@preserve` #15

Closed vtjnash closed 3 years ago

vtjnash commented 3 years ago

Currently the GC.@preserve calls are on a Ptr{*} value, which isn't actually a GC-tracked mutable-type, so are just ignored. I'm not sure the actual intent of them? It looks like they can be safely deleted.

helgee commented 3 years ago

These will be the remaining calls to GC.@preserve once #16 is merged. Do they look valid?

src/t.jl
232:    GC.@preserve out unsafe_string(pointer(out))

typeof(out) == Cstring

src/cells.jl
38:data_ptr(::Type{SpiceChar}, data, length) = GC.@preserve data pointer(data, CTRLSZ * length + 1)
39:data_ptr(::Type{T}, data, _) where {T} = GC.@preserve data pointer(data, CTRLSZ + 1)
50:        self.cell.base = GC.@preserve self pointer(self.data, 1)
182:    cell_copy.cell.base = GC.@preserve cell_copy pointer(cell_copy.data, 1)

typeof(data) == Array{T,N} where {T,N}

vtjnash commented 3 years ago

The former seems like it should call String(out), the later are invalid.

helgee commented 3 years ago

This codebase is now 100% preservative free 😬 Thanks for bringing it up 👍

vtjnash commented 3 years ago

Aright, but now you need to add correct ones back also: any place you use the cell field you need to surround with @GC.preserve data, to ensure the data field is still valid.

helgee commented 3 years ago

I was afraid you'd say that...

So, like this for example?

function bltfrm(frmcls)
    idset = SpiceIntCell(256)
    GC.@preserve idset.data begin
        ccall((:bltfrm_c, libcspice), Cvoid, (SpiceInt, Ref{Cell{SpiceInt}}), frmcls, idset.cell)
    end
    handleerror()
    idset
end

For my education, idset.cell gets passed to C and contains pointers to idset.data. Thus idset.data needs to be preserved. Correct?

vtjnash commented 3 years ago

That is correct