-
Notifications
You must be signed in to change notification settings - Fork 55
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
Array(T, dims...) to Array{T}(dims...) (ref JuliaLang/julia#19989) #116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! If my understanding is correct, in some cases the compiler might prefer including the number of dimensions in the curly braces (e.g. Array{T, length(d)}(d...)
) but I don't know when or how much that matters.
@@ -203,7 +203,7 @@ function jldopen(filename::AbstractString, rd::Bool, wr::Bool, cr::Bool, tr::Boo | |||
if sz < 512 | |||
error("File size indicates $filename cannot be a Julia data file") | |||
end | |||
magic = Array(UInt8, 512) | |||
magic = Array{UInt8}(512) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best to also specify dimensionality Array{UInt8,1}
or Vector{UInt8}
when known
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for the obvious Vector
/Matrix
cases.
There are still a couple of functions floating around with the argument dims::@compat Tuple{Vararg{Int}}
, which I could rewrite as dims::NTuple{N,Int}
so that I could use N
in the associated array constructors. I held off as I wasn't sure if it was necessary.
@@ -394,7 +394,7 @@ function read(obj::JldDataset) | |||
end | |||
if exists(obj, "dims") | |||
dims = a_read(obj.plain, "dims") | |||
return Array(T, dims...) | |||
return Array{T}(dims...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably no need for ...
@@ -643,7 +643,7 @@ end | |||
grefname = name(gref) | |||
try | |||
# Write the items to the reference group | |||
refs = Array(HDF5ReferenceObj, size(data)...) | |||
refs = Array{HDF5ReferenceObj}(size(data)...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same probably no need for ...
hmm all the tests are failing, what changed? |
oh nvm I think I understand why now, will submit patch |
whoops, should have checked ci on releases, my mistake for merging. was assuming jrevels wouldn't encourage merging something that wasn't ready yet. thanks in advance for any fixes @musm. |
patch included in #117 |
My bad, I missed the failure myself. |
I did this with a quick regex, so this should definitely be skimmed before merging
ref JuliaLang/julia#19989