Clean up string return values - #979
Conversation
| written, *excluding* the terminating NUL. Open MPI's `MPI_Get_library_version` | ||
| includes it, so trim instead of letting a NUL into the string. | ||
| """ | ||
| function _string_from_buffer(buf::Vector{UInt8}, len::Integer) |
There was a problem hiding this comment.
I'm mildly sure Base has something for this.
There was a problem hiding this comment.
Apparently Base doesn't for Vector{UInt8}. We can convert to String first, then strip (which gives a SubString), then maybe convert to String again?
There was a problem hiding this comment.
We can use chopsuffix which would remove at most one NUL. That's probably good enough.
There was a problem hiding this comment.
Doesn't
GC.@preserve buf unsafe_string(pointer(buf, len))do what you want? May still need to clamp len though.
There was a problem hiding this comment.
unsafe_string would ignore everything after the first NUL. I'm looking for a function that only removes a trailing NUL, if one is there.
Yes, clamping would be necessary anyway.
There was a problem hiding this comment.
I am not sure I understand? When would a NUL occur in a valid string?
There was a problem hiding this comment.
unsafe_string without a length argument would be unsafe because the buffer might not be NUL terminated.
unsafe_string with a length argument doesn't stop at NUL, it uses exactly that many bytes.
I think the current approach is good.
| lenref = Ref{Cint}() | ||
| API.MPI_Type_get_name(datatype, buffer, lenref) | ||
| return String(resize!(buffer, lenref[])) | ||
| return _string_from_buffer(buffer, lenref[]) |
There was a problem hiding this comment.
This looks like an upstream bug.
There was a problem hiding this comment.
It does, doesn't it? But Open MPI is so widely used that we should introduce a work-around, especially since all C/C++ users will never have notices this bug.
There was a problem hiding this comment.
I filed an upstream report at open-mpi/ompi#14411.
Open MPI sometimes include a trailing NUL into string values it returns. C doesn't see this. Remove all trailing NULs explicitly.
chopsuffix returns a SubString{String}, so Get_processor_name,
Get_library_version, error_string and get_name stopped returning a
String. test_misc.jl asserts `MPI.Get_processor_name() isa String`,
which failed on every CI job.
The resize!/findlast form also drops *all* trailing NULs, where
chopsuffix removed only one.
Documenter checks all docstrings in the MPI module against the manual and errors on ones with no @docs entry, failing docs-build. This is an internal helper, so make it a plain comment instead.
0aa804a to
26fadc9
Compare
3 participants
Open MPI sometimes include a trailing NUL into string values it returns. C doesn't see this. Remove all trailing NULs explicitly.