Skip to content

Conversation

timholy
Copy link
Member

@timholy timholy commented Jun 23, 2018

Adding S<:Tuple ensured a method would be higher in the methodtable.

Adding `S<:Tuple` ensured a method would be higher in the methodtable.
@coveralls
Copy link

coveralls commented Jun 23, 2018

Coverage Status

Coverage increased (+0.004%) to 88.3% when pulling 59791a5 on teh/errmsg into 5a9c7f7 on master.

@timholy
Copy link
Member Author

timholy commented Jun 23, 2018

Femtocleaner has a number of useful suggestions. OK to accept? Or would you rather keep this tiny?

@codecov-io
Copy link

codecov-io commented Jun 23, 2018

Codecov Report

Merging #448 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #448      +/-   ##
=========================================
+ Coverage   88.29%   88.3%   +<.01%     
=========================================
  Files          36      36              
  Lines        2982    2983       +1     
=========================================
+ Hits         2633    2634       +1     
  Misses        349     349
Impacted Files Coverage Δ
src/traits.jl 91.83% <100%> (+0.17%) ⬆️
src/SVector.jl 96.42% <100%> (ø) ⬆️
src/SizedArray.jl 94.11% <100%> (ø) ⬆️
src/FixedSizeArrays.jl 41.66% <100%> (ø) ⬆️
src/SDiagonal.jl 92.18% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a9c7f7...59791a5. Read the comment docs.

@andyferris
Copy link
Member

Cool, thanks Tim!

Yes I generally love femtocleaner- bring it on :)

@timholy timholy merged commit 4b1afb6 into master Jun 24, 2018
@timholy timholy deleted the teh/errmsg branch June 24, 2018 13:37
Size(a::T) where {T<:AbstractArray} = Size(T)
Size(::Type{<:StaticArray{S}}) where {S} = Size(S)
end
Size(::Type{SA}) where {SA <: StaticArray{S}} where {S<:Tuple} = Size(S) # S defined as a Tuple
Copy link
Member

@fredrikekre fredrikekre Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives UndefVarError: S not defined:

julia> convert(SVector, [1,2,3])
ERROR: UndefVarError: S not defined
Stacktrace:
 [1] Size(::Type{SArray{Tuple{S},T,1,S} where T where S}) at /Users/Fredrik/.julia/packages/StaticArrays/Jd49/src/traits.jl:86
 [2] length(::Type{SArray{Tuple{S},T,1,S} where T where S}) at /Users/Fredrik/.julia/packages/StaticArrays/Jd49/src/abstractarray.jl:2
 [3] convert(::Type{SArray{Tuple{S},T,1,S} where T where S}, ::Array{Int64,1}) at /Users/Fredrik/.julia/packages/StaticArrays/Jd49/src/convert.jl:22
 [4] top-level scope at none:0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. We tested this with 0.6, where it works as intended.

I wonder if this is actually a dispatch bug in 0.7. That line explicitly requires that S<:Tuple, and when S is undefined I don't see how that statement could be true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, it should work...

julia> abstract type AType{T} end

julia> foo(::Type{A}) where A<:AType = "parameter missing"
foo (generic function with 1 method)

julia> foo(::Type{A}) where A<:AType{T} where T<:Tuple = "parameter present"
foo (generic function with 2 methods)

julia> struct C{T} <: AType{T} end

julia> foo(C)
"parameter missing"

julia> foo(C{Tuple{1,2}})
"parameter present"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't what the exact problem is here, but note that, continuing with your definitions:

julia> foo(C{Union{T,Tuple}} where T <: Tuple)
"parameter present"

julia> foo(::Type{A}) where A<:AType{T} where T<:Tuple = "parameter present: $T"
foo (generic function with 2 methods)

julia> foo(C{Union{T,Tuple}} where T <: Tuple)
ERROR: UndefVarError: T not defined

So it's always safer to add an @isdefined T conditional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #452 to track this.

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 this pull request may close these issues.

6 participants