JuliaAstro / FITSIO.jl

Flexible Image Transport System (FITS) file support for Julia
http://juliaastro.org/FITSIO.jl/
MIT License
55 stars 29 forks source link

Pull request/7f281431 #21

Closed emmt closed 9 years ago

emmt commented 9 years ago
coveralls commented 9 years ago

Coverage Status

Coverage decreased (-7.86%) to 53.79% when pulling 7f281431e03371a72909f5b82feb2e5b56be875e on emmt:pull-request/7f281431 into 3001911efef010c799eee125f484082ba8d3e5a2 on JuliaAstro:master.

kbarbary commented 9 years ago

These look good, as discussed in emmt/OIFITS.jl#1... merging!

kbarbary commented 9 years ago

In this block:

for (T, code) in ((Uint8,       11),
                  (Int8,        12),
                  (Bool,        14),
                  (String,      16),
                  ...
                  (Complex128, 163))
    local value = cint(code)
    @eval begin
        if $T == String
            _cfitsio_datatype{T<:$T}(::Type{T}) = $value
        else
            _cfitsio_datatype(::Type{$T}) = $value
        end
    end
end

can we change String to ASCIIString and then simplify the loop body to the following?

@eval _cfitsio_datatype(::Type{$T}) = cint(code)

If so, I can make the change.

emmt commented 9 years ago

Le 21/03/2015 19:20, Kyle Barbary a écrit :

In this block:

for (T, code)in ((Uint8,11), (Int8,12), (Bool,14), (String,16), ... (Complex128,163)) local value= cint(code) @eval begin if $T== String _cfitsio_datatype{T<:$T}(::Type{T})= $value else _cfitsio_datatype(::Type{$T})= $value end end end

can we change |String| to |ASCIIString| and then simplify the loop body to the following?

@eval _cfitsio_datatype(::Type{$T})= cint(code)

If so, I can make the change.

— Reply to this email directly or view it on GitHub https://github.com/JuliaAstro/FITSIO.jl/pull/21#issuecomment-84413428.

Yes you can almost do that except keep the evaluation of the value outside of the macro (to speed up resulting code). So you should have something like:

for (T, code) in ((Uint8, 11), (Int8, 12), (Bool, 14), (ASCIIString, 16), ... (Complex128, 163)) local value = cint(code) @eval _cfitsio_datatype(::Type{$T}) = $value end

perhaps $(cint(code)) does the same, in which case:

for (T, code) in ((Uint8, 11), (Int8, 12), (Bool, 14), (ASCIIString, 16), ... (Complex128, 163)) @eval _cfitsio_datatype(::Type{$T}) = $(cint(code)) end

will be fine

kbarbary commented 9 years ago

Ah, yes, I should have had $code rather than code. But note that there's no performance penalty to

@eval _cfitsio_datatype(::Type{$T}) = cint($code)

Julia evaluates the constant expression cint($code) at compile time; there's no runtime penalty. For example:

julia> T = Int8; code = 11

julia> @eval f1(::Type{$T}) = int32($code)

julia> code_int32 = int32(code)
11

julia> @eval f2(::Type{$T}) = $code_int32

f1 has an int32 function call, f2 just has a constant, yet the native code is exactly the same:

julia> code_native(f1, (Type{Int8},))
    .text
Filename: none
Source line: 1
    push    RBP
    mov RBP, RSP
    mov EAX, 11
Source line: 1
    pop RBP
    ret

julia> code_native(f2, (Type{Int8},))
    .text
Filename: none
Source line: 1
    push    RBP
    mov RBP, RSP
    mov EAX, 11
Source line: 1
    pop RBP
    ret
emmt commented 9 years ago

OK you are right ;-)

Le 23/03/2015 16:08, Kyle Barbary a écrit :

Ah, yes, I should have had |$code| rather than |code|. But note that there's no performance penalty to

@eval _cfitsio_datatype(::Type{$T})= cint($code)

Julia evaluates the constant expression |cint($code)| at compile time; there's no runtime penalty. For example:

julia> T= Int8; code= 11

julia> @eval f1(::Type{$T})= int32($code)

julia> code_int32= int32(code) 11

julia> @eval f2(::Type{$T})= $code_int32

|f1| has an int32 function call, |f2| just has a constant, yet the native code is exactly the same:

julia> code_native(f1, (Type{Int8},)) .text Filename: none Source line: 1 push RBP mov RBP, RSP mov EAX,11 Source line: 1 pop RBP ret

julia> code_native(f2, (Type{Int8},)) .text Filename: none Source line: 1 push RBP mov RBP, RSP mov EAX,11 Source line: 1 pop RBP ret

— Reply to this email directly or view it on GitHub https://github.com/JuliaAstro/FITSIO.jl/pull/21#issuecomment-85041321.

                         Eric THIEBAUT

CRAL / Observatoire de Lyon | eric.thiebaut@univ-lyon1.fr 9, avenue Charles Andre | +33-(0)4 78 86 85 48 F-69561 Saint Genis Laval Cedex | +33-(0)4 78 86 83 86 (fax) http://cral.univ-lyon1.fr/labo/perso/eric.thiebaut/