Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Minimal changes to enable import on v0.6 #75

Closed
wants to merge 3 commits into from

Conversation

rdeits
Copy link
Contributor

@rdeits rdeits commented Feb 14, 2017

This is not ready for merge yet, but I just wanted to get a sense of what would be required to even import GeometryTypes in Julia v0.6-dev.

The tests don't all pass, because there seem to be issues with FixedSizeArrays fsa_abstract() like:

type UnionAll has no field name
  Stacktrace:
   [1] fsa_abstract(...) at /Users/rdeits/.julia/v0.6/FixedSizeArrays/src/core.jl:44
   [2] ndims(...) at /Users/rdeits/.julia/v0.6/FixedSizeArrays/src/core.jl:30
   [3] Type(...) at /Users/rdeits/.julia/v0.6/FixedSizeArrays/src/constructors.jl:64
   [4] macro expansion at /Users/rdeits/.julia/v0.6/GeometryTypes/test/gjk.jl:9 [inlined]

but this at least fixes failures on import.

The primary change is that in Julia v0.6, if you have an abstract type:

abstract Foo{T}

then you can't subtype from Foo (without the parameter). So this is an error:

abstract Bar <: Foo

but this is still OK:

abstract Bar{T} <: Foo{T}

See: JuliaLang/julia#19414

The fix was to explicitly include the N and T parameters in the various Mesh types so that we can have abstract AbstractMesh{N, T, VertT, FaceT} <: AbstractGeometry{N, T} instead of abstract AbstractMesh{VertT, FaceT} <: AbstractGeometry

Now that we have triangular dispatch in v0.6, it might be possible to do this in a more clever way. I'm not sure yet.

@rdeits
Copy link
Contributor Author

rdeits commented Feb 14, 2017

Is moving to StaticArrays still in the cards? That might be the best way to improve v0.6 compatibility.

@SimonDanisch
Copy link
Member

Yes moving to StaticArrays is the way forward!

@rdeits
Copy link
Contributor Author

rdeits commented Feb 14, 2017

Is that something you'd like help with?

@SimonDanisch
Copy link
Member

Thanks Robin! Yes if you want to get into this, you could start here:
#76
Didn't notice, that it wasn't actually a pull request up to now.
You can try out if that branch works for you and then we can go from there ;)
Although I believe, the main problem is getting this back to work on 0.6:
https://github.com/JuliaArrays/StaticArrays.jl/blob/master/src/StaticArrays.jl#L54

@rdeits
Copy link
Contributor Author

rdeits commented Feb 16, 2017

Great! I'll take a look over at #76.

@rdeits rdeits closed this Feb 16, 2017
@rdeits
Copy link
Contributor Author

rdeits commented Feb 16, 2017

@SimonDanisch I think the PR at #76 has the wrong commits attached. It doesn't look like it has any of the StaticArrays changes.

Anyway, I've started looking into this. It'll be a while before I really understand what needs to be done, but I've at least managed to uncover something weird that will have to be resolved to fix v0.6 compatibility: JuliaArrays/StaticArrays.jl#106

@SimonDanisch
Copy link
Member

Oh yeah, true! but from what I remember, I tried to keep the changes in GeometryTypes itself minimal.
And the biggest change (changing the face type) is already merged, if I remember correctly...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants