Use new where syntax in misc files #21428

musm · 2017-04-18T01:33:32Z

No description provided.

tknopp · 2017-04-18T15:23:12Z

Out of curious: Is this now the recommended form for parameterized functions?

yuyichao · 2017-04-18T15:34:00Z

The old version might be deprecated/replaced in 1.0 so that it can be used to define outer constructors.

tknopp · 2017-04-18T15:38:07Z

This is a pretty fundamental change. The old syntax was pretty readable IMHO. I thought that where is only required for complex relations that could not be expressed before.

musm · 2017-04-18T15:39:22Z

This is a pretty fundamental change. The old syntax was pretty readable IMHO. I thought that where is only required for complex relations that could not be expressed before.

The new syntax helps clarify parameterized function and parameterized constructors and IMO is a lot more clear and less confusing now.

tknopp · 2017-04-18T15:47:10Z

Could you give a link where this has been decided? In my point of view the former syntax was clear and readable, whereas the trailing notation requires looking back and forth (i.e. is less readable)

musm · 2017-04-18T15:55:20Z

Could you give a link where this has been decided? In my point of view the former syntax was clear and readable, whereas the trailing notation requires looking back and forth (i.e. is less readable)

Very heavily discussed here #18457

The new rule is a lot more simple: if there is a where then {T} is now a literal; e.g. things such as the following are a lot less confusing in 0.6 with the where syntax

type X{T}; end
X{T}(::T) = X{T}()

type Y{T}; end
Y{T}(::T) where {T} = Y{T}()

JeffBezanson · 2017-04-18T15:56:53Z

What does the following definition do?

Complex{Float64}(x) = ...

It shouldn't be controversial that this looks like it defines a constructor for the type Complex{Float64}, with one argument of any type. But it doesn't do that --- it defines a constructor for the type Complex, with a static parameter called Float64 that doesn't occur anywhere, so the method is not actually callable. This syntax was pretty clearly a mistake.

See discussion starting here: #10146 (comment)
Also issue #11310 which has most of the discussion
A bit of #8135

tknopp · 2017-04-18T16:02:49Z

I partly understand this issue within constructors. But this is a quite local issue in contrast to parametrized functions that occur a lot more often than constructors. I had a look at the above patch and it seems that 100% of the changes did not involve a constructor.

JeffBezanson · 2017-04-18T16:05:09Z

That is largely because this syntax change has already been done for constructors.

JeffBezanson · 2017-04-18T16:10:31Z

You're right that non-constructors occur much more often than constructors, but the syntax should still be consistent. We don't want A{B} to define a constructor for A{B} if A is a type, and otherwise introduce a static parameter. That would be both confusing and unnecessarily difficult to implement.

tknopp · 2017-04-18T16:32:27Z

I see the consistency issue here. I am not sure if this outweighs the regression in readability. One could also see the old syntax as a terse form for the new syntax and constructors are simply too complex to allow for a terse form.

JeffBezanson · 2017-04-18T16:50:10Z

Not everybody agrees that there is any regression in readability. For instance I found the examples in #11310 (comment) compelling in favor of the new syntax.

Arguably, the list of static parameters and their bounds is not the first thing you want to see. Both the old and new syntax group together (1) the arguments, (2) the static parameter bounds, so with either syntax you might have to look back and forth. There is also a new syntax that combines them:

f(x::Vector{<:Real}, y::Vector{<:Number}) = ...

In that case there is truly no looking back and forth.

Some also find that the where is a nice visual separation in what used to be a dense nest of punctuation, e.g. (::Type{T}){T}(arg) vs. (::Type{T})(arg) where T.

tknopp · 2017-04-18T16:57:39Z

ok, maybe I simply need to get used to that. Thanks for the explanation.

BobPortmann · 2017-04-19T01:49:53Z

So could the first line in this pull request

function indices{T,N}(A::AbstractArray{T,N}, d)

become

function indices(A::AbstractArray{<:Any,<:Any}, d)

instead of

function indices(A::AbstractArray{T,N}, d) where {T,N}

musm · 2017-04-19T01:59:08Z

No because we need N in the function https://github.com/JuliaLang/julia/pull/21428/files#diff-2264bb51acec4e7e2219a3cb1c733651R40

I suppose this would work
function indices(A::AbstractArray{<:Any,N}, d) where N
but I think it's more readable with both explicit in this particular instance ?

BobPortmann · 2017-04-19T02:19:01Z

Yes, of course

But I do agree with @tknopp that the new syntax is overly verbose for ordinary functions.

musm · 2017-04-19T02:49:12Z

I think once you get used to the syntax you start appreciating it, give it a shot first before dismissing it. It took me awhile to get used to the current syntax and how it works, but now it feels natural, so I think familiarity has strong impact on preference.
e.g. I really appreciate the new syntax for things such as
*(x::T,y::T) where {T<:Integer} #....
vs.
*{T<:Integer}(x::T,y::T) #....

Sacha0 · 2017-04-21T03:25:09Z

base/pair.jl

@@ -35,5 +35,5 @@ function convert(::Type{Pair{A,B}}, x::Pair) where A where B
    Pair{A, B}(convert(A, x[1]), convert(B, x[2]))
 end

-promote_rule{A1, B1, A2, B2}(::Type{Pair{A1, B1}}, ::Type{Pair{A2, B2}}) =
+promote_rule(::Type{Pair{A1, B1}}, ::Type{Pair{A2, B2}}) where {A1, B1, A2, B2} =


Extra spaces?

Sacha0 · 2017-04-21T03:33:32Z

base/dates/types.jl

@@ -298,7 +298,7 @@ Base.eltype{T<:Period}(::Type{T}) = T
 Base.promote_rule(::Type{Date}, x::Type{DateTime}) = DateTime
 Base.isless{T<:TimeType}(x::T, y::T) = isless(value(x), value(y))
 Base.isless(x::TimeType, y::TimeType) = isless(Base.promote_noncircular(x, y)...)
-=={T<:TimeType}(x::T, y::T) = ==(value(x), value(y))
+==(x::T, y::T) where {T<:TimeType} = ==(value(x), value(y))


(Unrelated, but the } = ==( tripped up my wetware parser. Perhaps infix == would be better?)

agreed, but I think this is better for a separate PR

Sacha0 · 2017-04-21T03:33:49Z

base/docs/utils.jl

@@ -72,7 +72,7 @@ print(io::IO, t::Text) = print(io, t.content)
 print(io::IO, t::Text{<:Function}) = t.content(io)
 show(io::IO, t::Text) = print(io, t)

-=={T<:Union{HTML, Text}}(t1::T, t2::T) = t1.content == t2.content
+==(t1::T, t2::T) where {T<:Union{HTML, Text}} = t1.content == t2.content


Extra space?

Sacha0 · 2017-04-21T03:44:30Z

base/test.jl

        a::StridedVecOrMat{S}, b::StridedVecOrMat{T},
        err = length(indices(a,1))^3*(eps(S)+eps(T))
-    )
+    ) where {S<:Real,T<:Real}


Indent seems odd?

Sacha0 · 2017-04-21T03:45:57Z

base/int.jl

@@ -25,17 +25,17 @@ const BitUnsigned64T = Union{Type{UInt8}, Type{UInt16}, Type{UInt32}, Type{UInt6

 ## integer comparisons ##

-<{T<:BitSigned}(x::T, y::T)  = slt_int(x, y)
+(<)(x::T, y::T) where {T<:BitSigned}  = slt_int(x, y)


Extra space or alignment here as well?

no this is supposed to be aligned with the code in the lines below

Sacha0 · 2017-04-21T03:49:44Z

base/promotion.jl

-promote_to_supertype{T<:Number,S<:Number}(::Type{T}, ::Type{S}, ::Type{T}) = (@_pure_meta; T)
-promote_to_supertype{T<:Number,S<:Number}(::Type{T}, ::Type{S}, ::Type{S}) = (@_pure_meta; S)
-promote_to_supertype{T<:Number,S<:Number}(::Type{T}, ::Type{S}, ::Type) =
+promote_to_supertype(::Type{T}, ::Type{T}, ::Type{T}) where {T<:Number          } = (@_pure_meta; T)


Perhaps the injection of spaces for alignment would be better between the } and =?

Sacha0 · 2017-04-21T04:00:04Z

base/linalg/lq.jl

@@ -137,15 +137,15 @@ end

 ## Multiplication by Q
 ### QB
-A_mul_B!{T<:BlasFloat}(A::LQPackedQ{T}, B::StridedVecOrMat{T})   = LAPACK.ormlq!('L','N',A.factors,A.τ,B)
+A_mul_B!(A::LQPackedQ{T}, B::StridedVecOrMat{T}) where {T<:BlasFloat}   = LAPACK.ormlq!('L','N',A.factors,A.τ,B)


Extra spaces?

Sacha0 · 2017-04-21T04:01:12Z

base/linalg/matmul.jl


-function dot{T<:BlasReal, TI<:Integer}(x::Vector{T}, rx::Union{UnitRange{TI},Range{TI}}, y::Vector{T}, ry::Union{UnitRange{TI},Range{TI}})
+function dot(x::Vector{T}, rx::Union{UnitRange{TI},Range{TI}}, y::Vector{T}, ry::Union{UnitRange{TI},Range{TI}}) where {T<:BlasReal, TI<:Integer}


Extra space?

Sacha0 · 2017-04-21T04:01:21Z

base/linalg/matmul.jl

@@ -56,7 +56,7 @@ function dot{T<:BlasReal, TI<:Integer}(x::Vector{T}, rx::Union{UnitRange{TI},Ran
    BLAS.dot(length(rx), pointer(x)+(first(rx)-1)*sizeof(T), step(rx), pointer(y)+(first(ry)-1)*sizeof(T), step(ry))
 end

-function dot{T<:BlasComplex, TI<:Integer}(x::Vector{T}, rx::Union{UnitRange{TI},Range{TI}}, y::Vector{T}, ry::Union{UnitRange{TI},Range{TI}})
+function dot(x::Vector{T}, rx::Union{UnitRange{TI},Range{TI}}, y::Vector{T}, ry::Union{UnitRange{TI},Range{TI}}) where {T<:BlasComplex, TI<:Integer}


Extra space?

Sacha0 · 2017-04-21T04:06:28Z

base/essentials.jl

@@ -133,18 +133,18 @@ function typename(a::Union)
 end
 typename(union::UnionAll) = typename(union.body)

-convert{T<:Tuple{Any,Vararg{Any}}}(::Type{T}, x::Tuple{Any, Vararg{Any}}) =
+convert(::Type{T}, x::Tuple{Any, Vararg{Any}}) where {T<:Tuple{Any,Vararg{Any}}} =


Inconsistent / extra space?

This space was already there but I might as well also fix this thanks

Sacha0

Much thanks for slogging through this @musm! Generally looks great.

(Abstracting from this PR, developing a consistent style guideline for when to use e.g. where {S,T} rather than where S where T in long-form method definitions involving more than one type parameter seems worthwhile. After reading through this diff, I lean towards using the compact form consistently.)

musm · 2017-04-21T04:21:55Z

(Abstracting from this PR, developing a consistent style guideline for when to use e.g. where {S,T} rather than where S where T in long-form method definitions involving more than one type parameter seems worthwhile. After reading through this diff, I lean towards using the compact form consistently.)

Agreed, although I don't have a strong pref on the convention, but that we should have some guideline

Also thank you for going through this. I pushed a commit to address reviews. Can you please take another look to give it a final g2g.

musm · 2017-04-21T04:23:35Z

base/test.jl

+                            imported::Bool = false,
+                            ambiguous_bottom::Bool = false,
+                            allow_bottom::Union{Bool,Void} = nothing)
+    if allow_bottom !== nothing


unrelated but shoould be !== not !=

musm · 2017-04-21T04:24:01Z

base/test.jl

-    if allow_bottom != nothing
+                            imported::Bool = false,
+                            ambiguous_bottom::Bool = false,
+                            allow_bottom::Union{Bool,Void} = nothing)


Is this how we break spacing ? I don't see anywhere else where we put the closing ) on a seperate line

Sacha0 · 2017-04-21T04:32:49Z

I pushed a commit to address reviews. Can you please take another look to give it a final g2g.

Checked out the touchup commit (#32) --- lgtm! :)

Should this be squashed?

Some squashing seems in order. Though how much squashing would be best I don't know. Perhaps someone wiser in this regard will weigh in?

musm · 2017-04-21T04:50:22Z

I think it might just make the most sense to squash all the commits, I can't see any benefit otherwise

musm · 2017-04-21T16:53:44Z

can we merge this now?

tkelman · 2017-04-23T01:08:44Z

The changes here in deprecated.jl made Lint.jl and LanguageServer.jl start failing their tests - https://github.com/JuliaCI/pkg.julialang.org/blob/3e484195ef2fcccae5f1447447c3590d142e570e/logs/Lint_0.6.log

That's a bug in those packages, but in the interest of not breaking things that were previously passing right before an RC, we should probably revert the part of this that changed deprecated.jl for now.

musm · 2017-04-23T01:10:45Z

cc @TotalVerb for Lint.jl
is there a log for LanguageServer ?

tkelman · 2017-04-23T01:12:51Z

https://github.com/JuliaCI/pkg.julialang.org/blob/3e484195ef2fcccae5f1447447c3590d142e570e/logs/LanguageServer_0.6.log but it's the exact same failure, I think it tries to Lint itself

TotalVerb · 2017-04-23T01:13:30Z

I'll have a fix in Lint. It's probably that lint is trying to parse Base's deprecated.jl file, and doesn't understand where.

musm · 2017-04-23T01:14:18Z

tahnks @TotalVerb !

musm force-pushed the where branch from 05858c4 to 9e30315 Compare April 18, 2017 16:06

musm force-pushed the where branch 4 times, most recently from 8f6eca2 to a961492 Compare April 19, 2017 00:51

musm force-pushed the where branch 8 times, most recently from aaa528e to 18ec763 Compare April 19, 2017 13:34

Sacha0 reviewed Apr 21, 2017

View reviewed changes

Sacha0 approved these changes Apr 21, 2017

View reviewed changes

musm force-pushed the where branch from b167cfc to c03f255 Compare April 21, 2017 04:20

musm commented Apr 21, 2017

View reviewed changes

Address review comments

a537847

musm force-pushed the where branch from c03f255 to a537847 Compare April 21, 2017 04:27

StefanKarpinski merged commit 5482f91 into JuliaLang:master Apr 21, 2017

musm deleted the where branch April 21, 2017 16:57

rofinn pushed a commit to invenia/julia that referenced this pull request Apr 21, 2017

Use new where syntax in misc files (JuliaLang#21428)

44887a2

This was referenced Apr 23, 2017

Fix Lint startup error on 0.6 tonyhffong/Lint.jl#224

Merged

Tag Lint v0.5.1 [https://github.com/tonyhffong/Lint.jl] JuliaLang/METADATA.jl#8965

Merged


		function dot{T<:BlasReal, TI<:Integer}(x::Vector{T}, rx::Union{UnitRange{TI},Range{TI}}, y::Vector{T}, ry::Union{UnitRange{TI},Range{TI}})
		function dot(x::Vector{T}, rx::Union{UnitRange{TI},Range{TI}}, y::Vector{T}, ry::Union{UnitRange{TI},Range{TI}}) where {T<:BlasReal, TI<:Integer}

Use new where syntax in misc files #21428

Use new where syntax in misc files #21428

Conversation

musm commented Apr 18, 2017

tknopp commented Apr 18, 2017

yuyichao commented Apr 18, 2017

tknopp commented Apr 18, 2017

musm commented Apr 18, 2017

tknopp commented Apr 18, 2017

musm commented Apr 18, 2017

JeffBezanson commented Apr 18, 2017

tknopp commented Apr 18, 2017 • edited Loading

JeffBezanson commented Apr 18, 2017

JeffBezanson commented Apr 18, 2017

tknopp commented Apr 18, 2017

JeffBezanson commented Apr 18, 2017

tknopp commented Apr 18, 2017

BobPortmann commented Apr 19, 2017

musm commented Apr 19, 2017

BobPortmann commented Apr 19, 2017

musm commented Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

musm commented Apr 21, 2017

Choose a reason for hiding this comment

musm Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Sacha0 commented Apr 21, 2017

musm commented Apr 21, 2017

musm commented Apr 21, 2017

tkelman commented Apr 23, 2017

musm commented Apr 23, 2017

tkelman commented Apr 23, 2017

TotalVerb commented Apr 23, 2017

musm commented Apr 23, 2017

tknopp commented Apr 18, 2017 •

edited

Loading

musm commented Apr 19, 2017 •

edited

Loading

musm Apr 21, 2017 •

edited

Loading