let ccall get the pointers using unsafe_convert

[visr]

Adress #347, alternative approach to #348, based on discussion in #347.

Still has a few errors, like this:

Expression: AG.toWKT(AG.createlinearring([1, 2, 3], [4, 5, 6])) == AG.toWKT(AG.createlinearring([1.0, 2.0, 3.0], [4.0, 5.0, 6.0])) == "LINEARRING (1 4,2 5,3 6)"
  MethodError: no method matching ArchGDAL.IGeometry{ArchGDAL.wkbLineString}(::ArchGDAL.Geometry{ArchGDAL.wkbLineString})

  Closest candidates are:
    ArchGDAL.IGeometry{T}(::Ptr{Nothing}) where T
     @ ArchGDAL D:\visser_mn\.julia\dev\ArchGDAL\src\types.jl:251

  Stacktrace:
   [1] createlinearring(xs::Vector{Int64}, ys::Vector{Int64})
     @ ArchGDAL D:\visser_mn\.julia\dev\ArchGDAL\src\ogr\geometry.jl:1751
   [2] macro expansion
     @ D:\visser_mn\.julia\juliaup\julia-1.10.0-latest+0.x64\share\julia\stdlib\v1.10\Test\src\Test.jl:477 [inlined]
   [3] macro expansion
     @ D:\visser_mn\.julia\dev\ArchGDAL\test\test_geometry.jl:339 [inlined]

So probably we need to add some constructors to handle this case. Not sure how to best do that safely, i.e. should we clone here? If not, how do we track lifetimes?

[yeesian]

Not sure how to best do that safely, i.e. should we clone here? If not, how do we track lifetimes?

Sorry for the slow response -- I do not understand enough about Julialang's GC behavior well enough to fully follow #347. E.g. why do we need to GC.@preserve obj f(obj.ptr) where we didn't previously have to, and why does the approach of using ccall here address the issue?

I've tried reading up about it (without much success) and maybe it's better for me to ask for help/clarification here.

[visr]

As far as I understand, we should've used either the preserve or unsafe_convert strategy already. Now with a more aggressive GC in the new Julia version our old approach causes actual segfaults, but the unsafe situation was always there.

The problem was, when we take out the obj.ptr field, the compiler cannot see anymore that obj itself is still in use, and might call the finalizer on it. We can stop it from doing that with the preserve annotation. But we can also pass obj to ccall, and ccall is implemented in a way that does a similar annotation for us already. So that is the easier option. To be able to use that we only need to help ccall with converting obj to ptr, and pass obj instead of obj.ptr to ccall.

https://docs.julialang.org/en/v1/base/base/#Base.GC.@preserve https://docs.julialang.org/en/v1/base/c/#Base.unsafe_convert

[yeesian]

Thank you for the explanation, really helpful! From what I understand (explained below), I agree with cloning here.

1. I think there's something weird about the definition of linearring methods in https://github.com/yeesian/ArchGDAL.jl/blob/7b7f559ab6b099298b0b92d675f562bbf5228a8f/src/ogr/geometry.jl#L1746-L1759

   (E.g. it's re-defining function createlinearring($(typedargs...)) for each possibility of $f1.) I have a feeling (i) $f1(Val{wkbLinearRing}()) should be unsafe_createlinestring(Val{wkbLinearRing}()), and (ii) those linearring function definitions should not be in the scope of for f in (:create, :unsafe_create).

2. I'm okay with it being less performant for now, and find it hard to reason about how to refactor it such that their implementations are still easy to reason about, given (i) the gotchas about linearring, (ii) the use of multi-dispatch on addpoint!(...), and (iii) what's explained about memory ownership across the ccall boundary.

[visr]

I'm not sure exactly what to change to the createlinearring functions. If anyone has ideas, feel free to push to this branch. Code like AG.createlinearring([1, 2, 3], [4, 5, 6]) now ultimately ends up with this error:

no method matching ArchGDAL.IGeometry{ArchGDAL.wkbLineString}(::ArchGDAL.Geometry{ArchGDAL.wkbLineString})

Is that not a kind of method that would be good to add? Reading the createlinearring functions, they are created by pushing points to an geometry and rewrapping that in a geometry with a different type parameter. I think that this is now failing because we don't directly pass pointers anymore, but a (possibly differently) typed (I)Geometry.

[rafaqz]

Just to explain the special casing a little GDAL swaps the wkbLinearRing type to wkbLineString after creation, so we have to do that too with these specialised methods.

Probably adding that method is a good fix? Although I haven't totally followed these changes.

[yeesian]

If anyone has ideas, feel free to push to this branch.

I'll give it a shot and let you know when I'm done, thanks!

[yeesian]

If anyone has ideas, feel free to push to this branch.

I'll give it a shot and let you know when I'm done, thanks!

Back to you @visr (you can review the summary of my changes here)

[visr]

Great, thanks. Your changes look good to me, and fix the issue.

The remaining test failures and error are unrelated to this PR, so I'd propose to handle them in a different PR.

Specifically the error that is only on nightly "type Stateful has no field taken" is this upstream issue: https://github.com/meggart/DiskArrays.jl/issues/80.

The rest is related to GDAL 3.6, #345, I updated that with some more info.