-
Notifications
You must be signed in to change notification settings - Fork 13
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
Define getindex(::InlineString, range)
to return InlineString
#44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great; thanks @nickrobinson251! The test failure is just Julia 1.3 being really old and having some other ambiguous range error. I'm happy to bump up to Julia 1.6 and we can do a new minor release.
getindex(::InlineString, range)
to return InlineStringgetindex(::InlineString, range)
to return InlineString
63fcf02
to
545f393
Compare
oooft, looks like v1.6 has the same ambiguity error as 1.3 😞 I think i'd rather us support 1.6 if we can, so i'll probably just oush a fix |
- Before on Julia v1.6: MethodError: getindex(::String255, ::UnitRange{Int64}) is ambiguous. Candidates: getindex(s::AbstractString, r::UnitRange{var"#s77"} where var"#s77"<:Integer) in Base at strings/substring.jl:255 getindex(s::InlineString, r::AbstractUnitRange{var"#s17"} where var"#s17"<:Integer) in InlineStrings at src/InlineStrings.jl:359
This is another case where we could be keeping things as
InlineString
rather than returning aSubString{InlineString}
(likechop
,chomp
,strip
etc) (xref #5)...but i don't love the (worse than linear) slowdown i'm seeing for larger InlineString types
I guess it makes sense since we're doing more work to construct a new InlineString (two
nextind
calls and then several bit-shifts) than the work involved in creating a SubString (which is basically a singlenextind
call)?Micro-benchmark
On
main
(returningSubString
)On this branch (returning
InlineString
)There's also
getindex(str, vec)
that allows selecting non-contiguous elements... so maybe if we add this we should add that too? (currently returnsSubString
)