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

Make concatenation of SubString{AnnotatedString} preserve annotations #51806

Merged

Conversation

tecosaur
Copy link
Contributor

SubStrings have been overlooked, and thanks to a few compiler quirks (relating to inlining and effect analysis), adding support for them is unfortunately a little more complicated than adding a || s isa SubString{<:AnnotatedString} clause thanks to the new generated runtime-checks.

To maintain the zero-overhead non-annotated code path, we need to implement a separate function _isannotated, which we also make use of to simplify the current join method.

@tecosaur tecosaur added the strings "Strings!" label Oct 21, 2023
@tecosaur tecosaur force-pushed the annot-string-substring-concat branch 2 times, most recently from a1ed366 to 8e2dd0d Compare October 21, 2023 11:55
@tecosaur
Copy link
Contributor Author

I must admit, I can't see what's going on with CI here.

@tecosaur tecosaur force-pushed the annot-string-substring-concat branch from 8e2dd0d to dc71228 Compare October 28, 2023 07:17
@tecosaur
Copy link
Contributor Author

Just pushed the use of _isannotated to simplify lpad and rpad.

@tecosaur
Copy link
Contributor Author

The CI failure seems unrelated:

make[1]: *** [stdlib/libLLVM_jll.release.image] Terminated: 15
Terminated: 15
🚨 Error: The command exited with status 143

@LilithHafner LilithHafner added the needs tests Unit tests are required for this change label Nov 9, 2023
@tecosaur tecosaur force-pushed the annot-string-substring-concat branch from f13ee94 to 91c4266 Compare November 9, 2023 02:29
SubStrings have been overlooked, and thanks to a few compiler
quirks (relating to inlining and effect analysis), adding support for
them is unfortunately a little more complicated than adding a
"|| s isa SubString{<:AnnotatedString}" clause thanks to the new
generated runtime-checks.

To maintain the zero-overhead non-annotated code path, we need to
implement a separate function _isannotated, which we also make use of to
simplify the current join method.

A SubString{AnnotatedString} value is added to the
Annotated{String,Char} concatenation test block.
@tecosaur tecosaur force-pushed the annot-string-substring-concat branch from 91c4266 to afeaee3 Compare November 9, 2023 02:33
@tecosaur
Copy link
Contributor Author

tecosaur commented Nov 9, 2023

Now with tests :)

@tecosaur tecosaur removed the needs tests Unit tests are required for this change label Nov 9, 2023
@tecosaur
Copy link
Contributor Author

tecosaur commented Nov 9, 2023

Ok, what's going on with CI? It seems consistently on the fritz again.

@tecosaur
Copy link
Contributor Author

I'd think this to be a pretty straightforward small improvement that could be quickly merged, is there anything holding this back?

@tecosaur tecosaur added the status: waiting for PR reviewer PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Dec 28, 2023
@tecosaur
Copy link
Contributor Author

It would be great if somebody with merge rights could spare the time to have a look at this PR.

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construction

if annotated
    annotatedstring
else
    string
end

doesn't feel natural in the generic-function/multiple-dispatch paradigm, which makes me averse to touching this. However, that aversion does not block this PR as this PR simply improves the situation by shifting some existing hardcoded || statements into generic functions (in order fix some substring functionality)

@LilithHafner LilithHafner removed the status: waiting for PR reviewer PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Jan 24, 2024
@LilithHafner LilithHafner changed the title Improve annotation *-concatenation behaviour Make concatenation of SubString{AnnotatedString} preserve annotations Jan 24, 2024
@LilithHafner LilithHafner merged commit ca7b9c3 into JuliaLang:master Jan 24, 2024
5 of 6 checks passed
@tecosaur
Copy link
Contributor Author

I get that aversion. However, the reason why "if annotated" now pops up in a few places is because it's extended a status quo of "hardcoded String". I'm not sure what a more idiomatic-looking (and non-breaking) approach would look like, and nobody really suggested one during the original review of this feature.

I do recall Valentin (Churavy) suggesting that annotatedstring should be merged with string (which I'd personally be quite happy with), but IIRC one or two other people in triage veto'd that.

@tecosaur tecosaur deleted the annot-string-substring-concat branch January 25, 2024 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants