JuliaStrings / InlineStrings.jl

Fixed-width string types for Julia
Other
45 stars 12 forks source link

Fixed method ambiguities for `Base.unsafe_convert()`, `Base.lstrip`, and `Base.rstrip` #70

Closed brainandforce closed 3 months ago

brainandforce commented 11 months ago

This doesn't fully solve #64, but it does solve two ambiguities: those between InlineStrings

unsafe_convert(::Type{Ptr{Int8}}}, x::Ref{T}) where T<:InlineString
unsafe_convert(::Type{Ptr{UInt8}}}, x::Ref{T}) where T<:InlineString

and Julia Base

unsafe_convert(::Type{P}, x::Ptr) where P<:Ptr
bkamins commented 11 months ago

could we add tests for these new methods?

codecov-commenter commented 11 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (bca08b0) 92.95% compared to head (7b26d49) 92.73%.

Files Patch % Lines
src/InlineStrings.jl 71.42% 2 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #70 +/- ## ========================================== - Coverage 92.95% 92.73% -0.22% ========================================== Files 2 2 Lines 681 688 +7 ========================================== + Hits 633 638 +5 - Misses 48 50 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

brainandforce commented 10 months ago

I've added some fixes and tests for new ambiguities I've discovered with Base.lstrip and Base.rstrip. However, I wasn't sure if it was meaningful to test methods containing a Ptr{<:InlineString} argument, the reason being that immutable objects do not have stable memory addresses.

In practice nobody should ever be constructing a Ptr{<:InlineString}. The method definition only exists to resolve the ambiguity.

mortenpi commented 3 months ago

Could this be merged and tagged?