Suppress warnings on julia v0.6 #26

dmbates · 2017-03-31T20:14:06Z

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.

ararslan · 2017-03-31T20:20:40Z

src/context.jl

    io::T                      # RDA input stream

    # RDA format properties
-    fmtver::UInt32             # format version
+    fmtver::Int32             # format version


Why change the internal field type? Would the format version ever be negative? If not, unsigned seems fine.

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.

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.

ararslan · 2017-03-31T20:20:53Z

src/context.jl

-
-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)


Should be 4 space indents instead of 2

I was switching back and forth between editing code and JSON files and must have left the indent at the wrong setting. Will fix.

ararslan · 2017-03-31T20:21:52Z

src/readers.jl

@@ -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)))


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.

Force of habit - In the old days we couldn't use Vector{Any}()

ararslan · 2017-03-31T20:22:44Z

REQUIRE

@@ -1,4 +1,4 @@
-julia 0.4
+julia 0.5


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.

Okay, will do.

ararslan

Looks alright to me

nalimilan · 2017-04-01T17:43:17Z

src/config.jl


-typealias RString UTF8String     # default String container for R string
-typealias Hash Dict{RString, Any}
+const RString = UTF8String     # default String container for R string


This should just be String with Julia ≥ 0.5. This probably means you can remove the UTF8String alias somewhere and replace all occurrences.

I propose const RString = String, i.e. keeping RString. So that we know that RString vars are unprocessed R Strings.

I will change it to const RString = String. I also changed the other two instances of UTF8String to String

nalimilan · 2017-04-01T17:45:34Z

src/context.jl

-
-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[])


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.

I'm not sure I understand. In what way would the inner constructor ensure validity of fields that the external constructor would not?

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.

I don't feel confident in making this change. Could someone else do it, please?

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.

nalimilan · 2017-04-01T17:46:03Z

src/io/ASCIIIO.jl

@@ -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)


@compat is no longer needed here and below.

nalimilan · 2017-04-01T17:48:23Z

src/io/XDRIO.jl

-    v = read(io.sub, Int32, n)
-    map!(ntoh, v, v)
-end
+readintorNA(io::XDRIO, n::RVecLength) = [readint32(io) for _ in 1:n]


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?

I was trying to avoid the separate map! and loop but I agree I made it worse, not better. I will revert that change.

alyst · 2017-04-01T23:36:16Z

src/io/XDRIO.jl

-
-    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))


Vector{UInt8}

alyst · 2017-04-01T23:41:16Z

src/io/XDRIO.jl

-    v = read(io.sub, UInt64, n)
-    reinterpret(Float64, map!(ntoh, v, v))
-end
+readfloatorNA(io::XDRIO, n::RVecLength) = [readfloat64(io) for _ in 1:n]


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.

alyst · 2017-04-01T23:48:40Z

src/readers.jl

@@ -231,7 +231,7 @@ function readbytecodeconsts(bctx::BytecodeContext)
            readitem(bctx.ctx)
        end
    end
-    return RList(v)
+    return RList(v, Hash())


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.

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

Was it with the old RVector inner constructor or with the new outer one?

It was with the old one. The new one fixed it.

alyst · 2017-04-01T23:51:46Z

src/sxtypes.jl

 end
+RVector{T}(v::Vector{T} = T[]) = RVector(v, Hash())


I would keep the inner ctor version as it wouldn't allow creation of RVectors with improperly initialized fields.

Okay. I see that making the change also cured the above issue.

alyst · 2017-04-05T18:26:54Z

src/sxtypes.jl

@@ -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)


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)

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.

You need (::Type{RVector{T,S}}){T,S}(v::Vector{T}=T[], attr::Hash=Hash()) = new{T,S}(v, attr)

tlnagy · 2017-04-09T20:43:41Z

Is this ready to merge? This package throws a bunch of warnings during Gadfly tests.

dmbates added 5 commits March 31, 2017 14:06

Build on @wookay's work for 0.6

cdb51fc

Use comprehensions

4a7bebb

Array construction syntax

f2e2a43

Somehow need an explicit Hash()

e194e2e

Don't test on 0.4 anymore

c933069

ararslan reviewed Mar 31, 2017

View reviewed changes

Incorporate suggestions by @ararslan

d4cbe72

ararslan approved these changes Mar 31, 2017

View reviewed changes

nalimilan reviewed Apr 1, 2017

View reviewed changes

alyst reviewed Apr 1, 2017

View reviewed changes

alyst requested changes Apr 1, 2017

View reviewed changes

wookay mentioned this pull request Apr 5, 2017

(WIP) preparation for Julia 0.6 #23

Closed

Suggestions from @alyst and @nalimilan

07755e5

alyst reviewed Apr 5, 2017

View reviewed changes

dmbates added 2 commits April 5, 2017 13:41

Revert inner constructor syntax change

aa29cfc

suppress warning on RVector CTOR

33c6ec3

alyst approved these changes Apr 5, 2017

View reviewed changes

ararslan merged commit 54d3385 into master Apr 10, 2017

ararslan deleted the v0.6 branch April 10, 2017 04:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress warnings on julia v0.6 #26

Suppress warnings on julia v0.6 #26

dmbates commented Mar 31, 2017

ararslan Mar 31, 2017

dmbates Mar 31, 2017

ararslan Mar 31, 2017

ararslan Mar 31, 2017

dmbates Mar 31, 2017

ararslan Mar 31, 2017

dmbates Mar 31, 2017

ararslan Mar 31, 2017

dmbates Mar 31, 2017

ararslan left a comment

nalimilan Apr 1, 2017

alyst Apr 1, 2017

dmbates Apr 5, 2017

nalimilan Apr 1, 2017

dmbates Apr 5, 2017

alyst Apr 5, 2017 •

edited

Loading

dmbates Apr 5, 2017

alyst Apr 5, 2017

nalimilan Apr 1, 2017

nalimilan Apr 1, 2017

dmbates Apr 5, 2017

alyst Apr 1, 2017

alyst Apr 1, 2017 •

edited

Loading

alyst Apr 1, 2017

dmbates Apr 5, 2017

alyst Apr 5, 2017

dmbates Apr 5, 2017

alyst Apr 1, 2017

dmbates Apr 5, 2017

alyst Apr 5, 2017

dmbates Apr 5, 2017

ararslan Apr 5, 2017

tlnagy commented Apr 9, 2017

Suppress warnings on julia v0.6 #26

Suppress warnings on julia v0.6 #26

Conversation

dmbates commented Mar 31, 2017

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

ararslan left a 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

alyst Apr 5, 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

alyst Apr 1, 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

tlnagy commented Apr 9, 2017

alyst Apr 5, 2017 •

edited

Loading

alyst Apr 1, 2017 •

edited

Loading