more conversions: StringView from StringView, Cstring from StringView #22
Conversation
| @@ -58,6 +59,7 @@ Base.unsafe_convert(::Type{Ptr{UInt8}}, s::DenseStringViewAndSub) = pointer(s) | |||
| Base.unsafe_convert(::Type{Ptr{Int8}}, s::DenseStringViewAndSub) = convert(Ptr{Int8}, pointer(s)) | |||
| Base.cconvert(::Type{Ptr{UInt8}}, s::DenseStringViewAndSub) = s | |||
| Base.cconvert(::Type{Ptr{Int8}}, s::DenseStringViewAndSub) = s | |||
| Base.cconvert(::Type{Cstring}, s::DenseStringViewAndSub) = s.data | |||
There was a problem hiding this comment.
TBH, I'm not that familiar with their interplay. cconvert is for C interop, that's why I define it.
There was a problem hiding this comment.
Hm, now I also think it shouldn't be added because Cstring has to be nul-terminated. Will remove it, sorry for the noise!
There was a problem hiding this comment.
Yes, probably we need to stick with the default conversion here.
There was a problem hiding this comment.
I don't understand how exactly, but it works correctly:
julia> sv = StringView([0x61, 0x62, 0x63])
"abc"
julia> @ccall strlen(sv::Cstring)::Csize_t
0x0000000000000003And as expected, much more efficient than converting to String:
julia> sv = StringView(codeunits("abc"^1000))
julia> @btime @ccall strlen($sv::Cstring)::Csize_t
1.437 μs (2 allocations: 3.12 KiB) # without this PR
38.424 ns (0 allocations: 0 bytes) # with this PR
0x0000000000000bb8 # result is the sameBoth of my PRs are motivated by enabling efficient C interop when strings are involved. StringViews seems a great fit for this.
There was a problem hiding this comment.
Ah, sorry, I misread your original question
Why not simply s?
Thought you are asking about convert vs cconvert.
Not just s because ccall does unsafe_convert(Cstring, x) then. It knows how to unsafe_convert Vector{UInt8} already, and that's what we return from cconvert here.
There was a problem hiding this comment.
I’m not convinced it is safe to assume that the data here is NUL-terminated. Have to go back and check what guarantees Julia provides
|
Add a couple of tests for the new methods? |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #22 +/- ##
==========================================
+ Coverage 75.05% 76.82% +1.76%
==========================================
Files 6 6
Lines 417 466 +49
==========================================
+ Hits 313 358 +45
- Misses 104 108 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
bump! |
No description provided.