Skip to content

Make SubString(::AbstractString) a noop construction #59606

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Make SubString(::AbstractString) a noop construction #59606

wants to merge 3 commits into from

Conversation

jakobnissen
Copy link
Member

@jakobnissen jakobnissen commented Sep 19, 2025 •
edited
Loading

This is always valid, and requires no checks or index computations.

Note that this PR is purely internal. There is a separate discussion to be had about an API for this (which I also think we should have), ref #59610

@jakobnissen
Make SubString(::AbstractString) a noop construction
35d3b77 
This is always valid, and requires no checks or index computations.
@jakobnissen jakobnissen added performance Must go faster strings "Strings!" labels Sep 19, 2025
@jakobnissen
Use new unsafe substring constructor
41cb151 
Previously, the way to create a substring without any index computation was to
use the strange :noshift constructor with `@inbounds`. However, the extra book-
keeping in that function would prevent inlining a noop constructor, despite
the book-keeping being disabled by `@inline`.

Instead, create a new unsafe constructor and use it where the @inbounds :noshift
constructor was used before.
@jakobnissen
Make name globally visible
d79efa1
@jakobnissen
Copy link
Member Author

AFAICT the CI failures are unrelated.

@nsajko

This comment was marked as resolved.

Sign in to view
@jakobnissen

This comment was marked as resolved.

Sign in to view
@nsajko

This comment was marked as resolved.

Sign in to view
@jakobnissen

This comment was marked as resolved.

Sign in to view
@nsajko
Copy link
Member

nsajko commented Sep 21, 2025

  1. The method exists already, with optional j, defined near to your change. SubString("", 1) returns as expected.

  2. What I had in mind is a fast path for i === 1 in the two-argument method, where the branch should get eliminated when i is constpropped/inferred as Core.Const(1). However I guess the branch could not be eliminated in the general case, so I guess you're right, and my suggestion is not good.

@tecosaur
Copy link
Member

Not to start a bikeshed, but in the interests of brevity, what do you think of _unsafe_substring instead of _unsafe_new_substring? I'm not sure that new adds that much information.

Oh also, we'll want to bump StyledStrings as part of this:

https://github.com/JuliaLang/StyledStrings.jl/blob/1aafc2f3abb6a977ee36a87206f9ce6446a8ae86/src/io.jl#L257

@jakobnissen jakobnissen mentioned this pull request Sep 26, 2025
Open
@jakobnissen
Copy link
Member Author

Closing in favor of #59663, which is similar but broader in scope since it also expands the API, as discussed on triage.

@oscardssmith oscardssmith removed their assignment Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

performance Must go faster strings "Strings!"

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jakobnissen @nsajko @tecosaur @oscardssmith