Skip to content
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

RFC: ByteOrder types for io #3633

Merged
merged 2 commits into from
Jul 24, 2013
Merged

RFC: ByteOrder types for io #3633

merged 2 commits into from
Jul 24, 2013

Conversation

simonbyrne
Copy link
Contributor

This adds ByteOrder types and read/write methods to make it easier to deal with different byte orderings.

The key advantage is arrays: previously I found myself using code such as:

x = read(f,Float64,100)
map!(ntoh,x)

Now it is possible to use

x = read(f,Float64,NetworkByteOrder,100)

It still needs some tests, documentation and exports, but in the meantime I would appreciate any feedback on the interface.

@ViralBShah
Copy link
Member

Cc: @loladiro

@Keno
Copy link
Member

Keno commented Jul 6, 2013

There needs to be a generic fallback that defaults to HostByteOrder

read(s::IO, ::Type{Bool}) = (read(s,Uint8)!=0)
read(s::IO, ::Type{Float32}) = box(Float32,unbox(Int32,read(s,Int32)))
read(s::IO, ::Type{Float64}) = box(Float64,unbox(Int64,read(s,Int64)))
read{B<:ByteOrder}(s::IO, ::Type{Float32}, ::Type{B}) = box(Float32,unbox(Int32,read(s,Int32,B)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while you're at it, this is much more nicely done using reinterpret instead of box/unbox.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know... reinterpret for single values make a lot more of operations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

julia> function bar(x)
       Intrinsics.box(Float64,Intrinsics.unbox(Int64,x))
       end
# methods for generic function bar
bar(x) at none:2

julia> disassemble(bar,(Int64,))

define double @julia_bar621(i64) {
top:
  %1 = bitcast i64 %0 to double, !dbg !5031
  ret double %1, !dbg !5031
}

julia> function foo(x)
       reinterpret(Float64,x)
       end
# methods for generic function foo
foo() at none:2
foo(x) at none:2

julia> disassemble(foo,(Int64,))

define double @julia_foo600(i64) {
top:
  %1 = bitcast i64 %0 to double, !dbg !4869
  ret double %1, !dbg !4869
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good :) I belief that reinterpretmakes conversion from value to array and get a value again by definition on Array. Based on function declaration at array.jl
Maybe it's something more to add into the style notes of the docs ;)

@Keno
Copy link
Member

Keno commented Jul 6, 2013

There is a pure-bits read/write method for arrays which will need to be adjusted.

@simonbyrne
Copy link
Contributor Author

I've added BitArray and Complex methods (and added the reinterpret as requested).

As for the fallback: there's one for Number (which currently are the only methods affected): I tried to create something more general but I kept getting ambiguous dispatch warnings. The code is a bit messy at the moment: i.e. for arrays I have to define methods with and without the ByteOrder argument. If anyone has any suggestions on how to clean it up, I would be grateful.

@simonbyrne
Copy link
Contributor Author

One other point: should HostByteOrder and ReverseByteOrder be the concrete types, and NetworkByteOrder and LittleByteOrder be aliases, or vice versa? The current choice was completely arbitrary, so if there is any reason to prefer one to the other, it can be easily changed.

@timholy
Copy link
Member

timholy commented Jul 24, 2013

This looks great. I'd say if you can resolve the merge conflict and perhaps write a few more tests (e.g., floating-point scalars, etc.), then we should merge this.

@pao
Copy link
Member

pao commented Jul 24, 2013

I suppose if this is merged StrPack should switch to this type. (Previously, it had been changed from a type to symbols due to the language zeitgeist...is the pendulum swinging back?)

Of course would also take advantage of the updated method signatures.

@simonbyrne
Copy link
Contributor Author

Okay, I've rebased to master, and added some more tests.

I'm still not sure about the interface

  • I would have liked to use keyword arguments, but this would require everyone else to define their read/write methods with them, otherwise the Array methods would break (including cases such as Uint8 where they're not needed). Any ideas?
  • The only other alternative I could think of was to define them as parametric types, i.e. write(io,NetworkByteOrder(4.0)), read(io,NetworkByteOrder{Float64}), but again this didn't seem to work for arrays.

I haven't really kept up with the current trends re: tags. The only other place in base where they seem to be widely used is sort.jl, but they do it slightly differently by exporting const instances. Would that be a better approach?

@timholy
Copy link
Member

timholy commented Jul 24, 2013

I think for low-level IO keyword args are not the right way to go. This leverages dispatch.

I think it's plenty good, and the issue about const instances can be addressed later if someone thinks it's important. Enough bikeshedding.

timholy added a commit that referenced this pull request Jul 24, 2013
@timholy timholy merged commit 900478c into JuliaLang:master Jul 24, 2013
@StefanKarpinski
Copy link
Member

I guess I'm a little late here, but I object to this interface. Primarily my objection is that byte order should be property of streams, not data, and certainly shouldn't normally be something that is specified on individual reads and writes. I'm also not super thrilled about the API of passing these network byte order objects as straggling arguments.

@simonbyrne
Copy link
Contributor Author

Well, what about if we use a type wrapper around IO types? Something like:

type NetworkByteOrder{T<:IO}
  io::T
end
type LittleByteOrder{T<:IO}
  io::T
end

read(NetworkByteOrder(io),Float64)

@StefanKarpinski
Copy link
Member

That seems much better to me. I think @JeffBezanson has some opinions here as well.

KristofferC pushed a commit that referenced this pull request Oct 17, 2023
Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: master
Julia branch: master
Old commit: b02fb9597
New commit: ffb6edf03
Julia version: 1.11.0-DEV
Pkg version: 1.11.0
Bump invoked by: @IanButterworth
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@b02fb95...ffb6edf

```
$ git log --oneline b02fb9597..ffb6edf03
ffb6edf03 cache pidlock tweaks (#3654)
550eadd7e Pin registry for MetaGraph tests (#3666)
ee39026b8 Remove test that depends on Random being in the sysimg (#3656)
561508db2 CI: Increase the CI timeout. Update actions. Fix double precompilation. (#3665)
7c7ed63b1 Remove change UUID script it should be uncessary on Julia v1.11-dev (#3655)
a8648f7c8 Precompile: Fix algorithmic complexity of cycle detection (#3651)
0e0cf4514 Switch datastructure Vector -> Set for algorithmic complexity (#3652)
894cc3f78 respect if load-time precompile is disabled (#3648)
3ffd1cf73 Make auto GC message use printpkgstyle (#3633)
```

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants