Skip to content
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

Move eachregion(::AnnotatedString) implementation to Base #57912

Merged
merged 2 commits into from
Mar 29, 2025

Conversation

topolarity
Copy link
Member

Excising part of #56194 on the way to reviving that PR.

Re-organizing these definitions - no changes to the code.
@topolarity topolarity added invalidations backport 1.12 Change should be backported to release-1.12 labels Mar 27, 2025
@topolarity topolarity requested a review from tecosaur March 27, 2025 20:45
@topolarity topolarity changed the title Move eachregion(::AnnotatedString) implementation to Base Move eachregion(::AnnotatedString) implementation to Base Mar 27, 2025
@topolarity topolarity force-pushed the ct/mv-eachregion branch 3 times, most recently from bafdc1b to c1522a3 Compare March 27, 2025 20:53
Base needs this functionality so that it can iterate its own
`AnnotatedString`s.
@nsajko
Copy link
Contributor

nsajko commented Mar 28, 2025

Is this PR supposed to prevent any invalidations? I can't find such an example. I tried things like this, but the result is the same on nightly and with this PR:

using StyledStrings: StyledStrings
using SnoopCompileCore: @snoop_invalidations
raw_invalidations = @snoop_invalidations begin
    using CSV: CSV 
end

@topolarity
Copy link
Member Author

Not directly - I added the label because it's part of a longer-term plan to resolve the type piracy in StyledStrings, which should remove many invalidations

@topolarity
Copy link
Member Author

topolarity commented Mar 29, 2025

I'm going to merge this, since it is unambiguously type-piracy for this to be defined outside of Base. There are still decisions to make w.r.t. solving the rest of the StyledStrings type piracy, but those can be tackled in separate PR's

@topolarity topolarity merged commit a5e0eab into JuliaLang:master Mar 29, 2025
7 checks passed
@tecosaur
Copy link
Contributor

Cool, as you say this is an early building block and a very straightforward change 👍.

KristofferC pushed a commit that referenced this pull request Mar 31, 2025
Excising part of #56194 on the
way to reviving that PR.

(cherry picked from commit a5e0eab)
@KristofferC KristofferC mentioned this pull request Mar 31, 2025
36 tasks
KristofferC pushed a commit that referenced this pull request Mar 31, 2025
Excising part of #56194 on the
way to reviving that PR.

(cherry picked from commit a5e0eab)
@KristofferC KristofferC mentioned this pull request Apr 4, 2025
30 tasks
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants