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

more detailed docstring for read! and write #49769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mgkuhn
Copy link
Contributor

@mgkuhn mgkuhn commented May 11, 2023

The old read! docstring failed to describe how many bytes the
function reads and what its return value is.

base/io.jl Outdated
read!(stream::IO, a::Ref)
read!(filename::AbstractString, a::Union{AbstractArray, Ref})

Read `sizeof(a)` bytes of binary data from an I/O stream or file,
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite right — the number of bytes is only sizeof(a) if isbitstype(eltype(a)).

A more general and accurate description would be:

Read `length(a)` consecutive values of type `eltype(a)` from an I/O stream or file into `a`, returning `a`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the function even work for non-isbits?

Copy link
Contributor Author

@mgkuhn mgkuhn May 12, 2023

Choose a reason for hiding this comment

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

@Seelengrab: If isbitstype(T)==false, then read!(..., a::AbstractArray{T,N}) just runs

for i in eachindex(a)
    a[i] = read(s, T)
end

i.e. falls back to whatever read() method was provided for type T, and the number of bytes read will depend on what that does. Also, it seems read!(..., a::Ref{T}) currently doesn't have such a fallback and therefore is only intended for isbitstype(T)==true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, that's what I mean - non-bitstypes can't really be read like that, because of the pointers contained within. They need to be properly deserialized and recreated in a more complicated manner.

The old `read!` docstring failed to describe how many bytes the
function reads and what its return value is.

The old `write` docstring didn't cover arrays.

I also documented `read!(..., Ref{T})`: I first wasn't sure if this
was meant to be documented, but then saw that it was already mentioned
in the docstring of `write`.
@mgkuhn mgkuhn changed the title more detailed docstring for read! more detailed docstring for read! and write May 12, 2023
@mgkuhn
Copy link
Contributor Author

mgkuhn commented May 12, 2023

I've now updated this docstring PR:

  • I've used for read! phrasing similar to what @stevengj suggested
  • I've added a note that read!(..., Ref{T}) is only intended for primitive types. I first wasn't sure whether it should be documented at all (as it seems an unsafe function when used with non-primitive types), but then saw that it was already mentioned in the docstring of write.
  • The write docstring now also covers writing arrays.

base/io.jl Show resolved Hide resolved
@brenhinkeller brenhinkeller added the docs This change adds or pertains to documentation label Aug 5, 2023
User-defined plain-data types without `write` methods can be written when wrapped in a `Ref`:
Arrays:
```jldoctest
julia> write(stdout, [0x41 0x42; 0x43 0x44], 0x0a)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
julia> write(stdout, [0x41 0x42; 0x43 0x44], 0x0a)
julia> write(stdout, UInt8['A' 'B'; 'C' 'D'], '\n')

Comment on lines +494 to +496
If `a` is an `Array` and `isbitstype(T)` is true, this function reads
`size(a)` bytes directly into memory. Otherwise, each element of
`a::AbstractArray{T}` is read with a call to `read(stream, T)`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If `a` is an `Array` and `isbitstype(T)` is true, this function reads
`size(a)` bytes directly into memory. Otherwise, each element of
`a::AbstractArray{T}` is read with a call to `read(stream, T)`.
If `isbitstype(T)` is true, this function reads `sizeof(a)` bytes
directly into memory. Otherwise, each element of `a` is populated
sequentially by a call to `read(stream, T)`.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if sizeof(a) is accurate here.
Base.sizeof is not defined for most AbstractArray and it falls back to Core.sizeof by default.

BTW, if we expect AbstractArrays to behave like Arrays, then the optimization for StridedArrays needs to check elsize(A) == elsize(Array{eltype(A)}) to avoid possible padding mismatch.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, maybe sizeof(T) * length(a) bytes would be better then? (more accurate yet would be Base.aligned_sizeof, but that seems a bit overly pedantic to me)

vtjnash added a commit that referenced this pull request Oct 30, 2023
Align the API for Ref with the new definition for AbstractArray (#49769)
and ensure this API does not accept raw Ptr as input.

Refs #42593
vtjnash added a commit that referenced this pull request Oct 30, 2023
Align the API for Ref with the new definition for AbstractArray (#49769)
and ensure this API does not accept raw Ptr as input.

Refs #42593
vtjnash added a commit that referenced this pull request Oct 30, 2023
Align the API for Ref with the new definition for AbstractArray (#49769)
and ensure this API does not accept raw Ptr as input.

Refs #42593
vtjnash added a commit that referenced this pull request Oct 31, 2023
Align the API for Ref with the new definition for AbstractArray (#49769)
and ensure this API does not accept raw Ptr as input.

Refs #42593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants