-
Notifications
You must be signed in to change notification settings - Fork 16
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
Suppress warnings on julia v0.6 #26
Conversation
src/context.jl
Outdated
io::T # RDA input stream | ||
|
||
# RDA format properties | ||
fmtver::UInt32 # format version | ||
fmtver::Int32 # format version |
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.
Why change the internal field type? Would the format version ever be negative? If not, unsigned seems fine.
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.
The constructor used readint32
, which caused an incompatibility. Because the only stream wrappers in use (ASCIIIO and XDRIO) have a readuint32 method, I will revert this change and instead change the constructor.
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.
Okay, well whatever you think is better. I was mostly curious as to why this was changed. The option that would cause the least visible breakage would be good.
src/context.jl
Outdated
|
||
RDAContext{T <: RDAIO}(io::T, kwoptions::Vector{Any}) = RDAContext{T}(io, kwoptions) | ||
function RDAContext{T<:RDAIO}(io::T, kwoptions::Vector{Any}=Any[]) | ||
fmtver = readint32(io) |
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.
Should be 4 space indents instead of 2
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.
I was switching back and forth between editing code and JSON files and must have left the indent at the wrong setting. Will fix.
src/readers.jl
Outdated
@@ -184,7 +184,7 @@ immutable BytecodeContext | |||
ctx::RDAContext # parent RDA context | |||
ref_tab::Vector{Any} # table of bytecode references | |||
|
|||
BytecodeContext(ctx::RDAContext, nrefs::Int32) = new(ctx, Array(Any, Int(nrefs))) | |||
BytecodeContext(ctx::RDAContext, nrefs::Int32) = new(ctx, Array{Any}(Int(nrefs))) |
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.
I'd make this Vector{Any}
instead of Array{Any}
, since the former will be a little easier on the compiler and is a little clearer.
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.
Force of habit - In the old days we couldn't use Vector{Any}()
@@ -1,4 +1,4 @@ | |||
julia 0.4 | |||
julia 0.5 |
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.
We'll also need to raise the minimum version of Compat in this file to 0.17 (I think that's it) to allow @compat abstract type
.
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.
Okay, will do.
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 alright to me
src/config.jl
Outdated
|
||
typealias RString UTF8String # default String container for R string | ||
typealias Hash Dict{RString, Any} | ||
const RString = UTF8String # default String container for R string |
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.
This should just be String
with Julia ≥ 0.5. This probably means you can remove the UTF8String
alias somewhere and replace all occurrences.
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.
I propose const RString = String
, i.e. keeping RString
. So that we know that RString
vars are unprocessed R Strings.
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.
I will change it to const RString = String
. I also changed the other two instances of UTF8String
to String
|
||
RDAContext{T <: RDAIO}(io::T, kwoptions::Vector{Any}) = RDAContext{T}(io, kwoptions) | ||
int2ver(v::Integer) = VersionNumber(v >> 16, (v >> 8) & 0xff, v & 0xff) | ||
function RDAContext{T<:RDAIO}(io::T, kwoptions::Vector{Any}=Any[]) |
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.
Maybe the old inner constructor was added in the first place to ensure the fields are valid? It so, you can keep it using the (::Type{RDAContext{T}}){T}(...) =
syntax.
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.
I'm not sure I understand. In what way would the inner constructor ensure validity of fields that the external constructor would not?
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.
Because when the inner ctor is explicitly defined, the default implicit inner ctor (which takes the value for every field) is not generated. So any creation of the object would have to go through the explicitly provided inner ctor. Inner Constructors in the Julia manual.
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.
I don't feel confident in making this change. Could someone else do it, please?
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.
I think this outer ctor version is fine. Initializing the fields from io
is not the inner ctor job. The only field that outer ctor should not manage is ref_tab
, but it's a minor issue.
src/io/ASCIIIO.jl
Outdated
@@ -4,7 +4,6 @@ | |||
type ASCIIIO{T<:IO} <: RDAIO | |||
sub::T # underlying IO stream | |||
|
|||
ASCIIIO(io::T) = new(io) | |||
@compat (::Type{ASCIIIO}){T<:IO}(io::T) = new{T}(io) |
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.
@compat
is no longer needed here and below.
src/io/XDRIO.jl
Outdated
v = read(io.sub, Int32, n) | ||
map!(ntoh, v, v) | ||
end | ||
readintorNA(io::XDRIO, n::RVecLength) = [readint32(io) for _ in 1:n] |
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.
I'm afraid the new version is less efficient since it allocates a copy for the new vector. What's the problem with the old version?
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.
I was trying to avoid the separate map!
and loop but I agree I made it worse, not better. I will revert that change.
src/io/XDRIO.jl
Outdated
|
||
XDRIO(io::T) = new(io, Array(UInt8, 1024)) | ||
@compat (::Type{XDRIO}){T <: IO}(io::T) = new{T}(io, Array(UInt8, 1024)) | ||
@compat (::Type{XDRIO}){T <: IO}(io::T) = new{T}(io, Array{UInt8}(1024)) |
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.
Vector{UInt8}
src/io/XDRIO.jl
Outdated
v = read(io.sub, UInt64, n) | ||
reinterpret(Float64, map!(ntoh, v, v)) | ||
end | ||
readfloatorNA(io::XDRIO, n::RVecLength) = [readfloat64(io) for _ in 1:n] |
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 as for readintorNA()
. AFAIR when I was testing different implementations, there was a big overhead of doing tiny reads vs doing one read for the whole array (although it was for Julia 0.3/0.4). I suggest to keep the old version.
src/readers.jl
Outdated
@@ -231,7 +231,7 @@ function readbytecodeconsts(bctx::BytecodeContext) | |||
readitem(bctx.ctx) | |||
end | |||
end | |||
return RList(v) | |||
return RList(v, Hash()) |
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.
Hash()
is not necessary since it's the default value for the RVector
inner ctor. The current variant is more robust to potential API changes.
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.
Without that Hash()
I get in version 0.6.0-pre.beta
julia6 test/runtests.jl
ERROR: LoadError: LoadError: MethodError: Cannot `convert` an object of type Array{RData.RSEXPREC,1} to an object of type RData.RVector{RData.RSEXPREC,0x13}
This may have arisen from a call to the constructor RData.RVector{RData.RSEXPREC,0x13}(...),
since type constructors fall back to convert methods.
Stacktrace:
[1] readbytecodeconsts(::RData.BytecodeContext) at /home/bates/.julia/v0.6/RData/src/readers.jl:234
[2] readbytecodecontents(::RData.BytecodeContext) at /home/bates/.julia/v0.6/RData/src/readers.jl:238
[3] readbytecode(::RData.RDAContext{RData.XDRIO{GZip.GZipStream}}, ::UInt32) at /home/bates/.julia/v0.6/RData/src/readers.jl:244
[4] readitem(::RData.RDAContext{RData.XDRIO{GZip.GZipStream}}) at /home/bates/.julia/v0.6/RData/src/readers.jl:314
[5] readclosure(::RData.RDAContext{RData.XDRIO{GZip.GZipStream}}, ::UInt32) at /home/bates/.julia/v0.6/RData/src/readers.jl:146
[6] readitem(::RData.RDAContext{RData.XDRIO{GZip.GZipStream}}) at /home/bates/.julia/v0.6/RData/src/readers.jl:314
[7] load(::FileIO.Stream{FileIO.DataFormat{:RData},GZip.GZipStream}, ::Array{Any,1}) at /home/bates/.julia/v0.6/RData/src/RData.jl:72
[8] gzopen(::RData.##4#5{Array{Any,1},FileIO.File{FileIO.DataFormat{:RData}}}, ::String) at /home/bates/.julia/v0.6/GZip/src/GZip.jl:268
[9] #load#3(::Array{Any,1}, ::Function, ::FileIO.File{FileIO.DataFormat{:RData}}) at /home/bates/.julia/v0.6/RData/src/RData.jl:46
[10] (::FileIO.#kw##load)(::Array{Any,1}, ::FileIO.#load, ::FileIO.File{FileIO.DataFormat{:RData}}) at ./<missing>:0
[11] #load#13(::Array{Any,1}, ::Function, ::String) at /home/bates/.julia/v0.6/FileIO/src/loadsave.jl:45
[12] (::FileIO.#kw##load)(::Array{Any,1}, ::FileIO.#load, ::String) at ./<missing>:0
[13] include_from_node1(::String) at ./loading.jl:539
[14] include(::String) at ./sysimg.jl:14
[15] include_from_node1(::String) at ./loading.jl:539
[16] include(::String) at ./sysimg.jl:14
[17] process_options(::Base.JLOptions) at ./client.jl:305
[18] _start() at ./client.jl:371
while loading /home/bates/.julia/v0.6/RData/test/RDA.jl, in expression starting on line 53
while loading /home/bates/.julia/v0.6/RData/test/runtests.jl, in expression starting on line 4
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.
Was it with the old RVector
inner constructor or with the new outer one?
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.
It was with the old one. The new one fixed it.
src/sxtypes.jl
Outdated
end | ||
RVector{T}(v::Vector{T} = T[]) = RVector(v, Hash()) |
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.
I would keep the inner ctor version as it wouldn't allow creation of RVectors
with improperly initialized fields.
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.
Okay. I see that making the change also cured the above issue.
src/sxtypes.jl
Outdated
@@ -111,8 +111,10 @@ end | |||
type RVector{T, S} <: RVEC{T, S} | |||
data::Vector{T} | |||
attr::Hash # collection of R object attributes | |||
|
|||
@compat RVector{T,S}(v::Vector{T} = T[], attr::Hash = Hash()) where {T,S} = new(v, attr) |
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.
AFAIU @compat
doesn't/cannot support 0.6 where
syntax on 0.5. But, in fact, you don't need explicit type parameters here as they would be taken from the type definition. I hope this would work
RVector(v::Vector{T} = T[], attr::Hash = Hash()) = new(v, attr)
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.
I reverted the change but now get a warning
WARNING: deprecated syntax "inner constructor RVector(...) around /home/bates/.julia/v0.6/RData/src/sxtypes.jl:115".
Use "RVector{T,S}(...) where {T,S}" instead.
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.
You need (::Type{RVector{T,S}}){T,S}(v::Vector{T}=T[], attr::Hash=Hash()) = new{T,S}(v, attr)
Is this ready to merge? This package throws a bunch of warnings during Gadfly tests. |
Building on the work by @wookay but allowing for this version to continue to be used with v0.5
I also tightened the code in a couple of places.