Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove all useless calls to GC.@preserve #15

Closed
vtjnash opened this issue Jun 3, 2021 · 6 comments · Fixed by #16
Closed

Remove all useless calls to GC.@preserve #15

vtjnash opened this issue Jun 3, 2021 · 6 comments · Fixed by #16

Comments

@vtjnash
Copy link

vtjnash commented Jun 3, 2021

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
Copy link
Member

helgee commented Jul 28, 2021

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
Copy link
Author

vtjnash commented Jul 28, 2021

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

@helgee
Copy link
Member

helgee commented Jul 29, 2021

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

@vtjnash
Copy link
Author

vtjnash commented Jul 29, 2021

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
Copy link
Member

helgee commented Jul 29, 2021

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?

@helgee helgee reopened this Jul 29, 2021
@vtjnash
Copy link
Author

vtjnash commented Jul 29, 2021

That is correct

@helgee helgee closed this as completed Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants