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

Remove implicit conversions from String, Char16String and CharString to data pointers. #101293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Jan 8, 2025

This is mostly a sanity change to avoid accidental conversions leading to unexpected behavior.

I have not tested how many callers are affected (e.g. String s = char_string).
All that are will experience a noticeable improvement of speed because strlen does not have to be called.

Notes on implicitness

I have made conversions from String-likes to StrRange implicit to aid the transition.
I thought long and hard about whether I want to 'contribute' another implicit conversion to the codebase, seeing as though they are already leading to quite a lot of problems.

However, I think conversion to StrRange is one of the rare cases where it actually is warranted:

  • The conversion does not lose metadata about the contents (pointer and length).
  • No data is allocated, and the conversion is trivial.

The only gotcha for this conversion is really that it may lead to ambiguous calls, and it may lead to unintentional loss of ownership. However, both are true about the const char * conversions, so it's an upgrade all around.

Behavior changes

There are a few behavior changes that may lead to problems (though they are unlikely to). If problems are experienced somewhere, it's likely the caller should have been more explicit from the start, and should be addressed to do so.

Especially, callers will change behavior when encountering a \0: Instead of cutting the string short, they will add the character to the middle of the string. In terms of UTF-32, the new behavior is correct. Still, if unexpected behavior is observed, it should be evaluated on a case-by-case basis - most likely, the previous behavior was incorrect other behavior should be adjusted to fit.

@Ivorforce Ivorforce requested review from a team as code owners January 8, 2025 15:42
@Ivorforce Ivorforce force-pushed the string-to-pointer-conversion branch from 7487048 to 72282c6 Compare January 8, 2025 15:43
editor/export/codesign.cpp Show resolved Hide resolved
editor/export/codesign.cpp Show resolved Hide resolved
@Ivorforce Ivorforce force-pushed the string-to-pointer-conversion branch from 72282c6 to a954b0f Compare January 8, 2025 15:52
@Ivorforce Ivorforce requested a review from a team as a code owner January 8, 2025 15:52
@Ivorforce Ivorforce force-pushed the string-to-pointer-conversion branch from a954b0f to f509141 Compare January 8, 2025 16:48
@Ivorforce Ivorforce requested a review from a team as a code owner January 8, 2025 16:48
core/io/plist.cpp Outdated Show resolved Hide resolved
core/io/plist.cpp Outdated Show resolved Hide resolved
@Ivorforce Ivorforce force-pushed the string-to-pointer-conversion branch from f509141 to ddbd8c2 Compare January 8, 2025 18:01
@Ivorforce Ivorforce force-pushed the string-to-pointer-conversion branch from ddbd8c2 to 57cf915 Compare January 8, 2025 18:57
@Ivorforce Ivorforce requested a review from a team as a code owner January 8, 2025 18:57
@Ivorforce Ivorforce force-pushed the string-to-pointer-conversion branch 2 times, most recently from 4687c61 to 42a3ff5 Compare January 8, 2025 19:11
@Ivorforce
Copy link
Contributor Author

Sorry about the repeated pushes!! I should really learn to test this kinda change on my own repo to get other platforms to pass before I make a PR 🙃

@bruvzg
Copy link
Member

bruvzg commented Jan 9, 2025

I assume this is superseded by #101304, which include all changes from this PR.

@Ivorforce Ivorforce force-pushed the string-to-pointer-conversion branch 2 times, most recently from 2a2ae23 to e2f7611 Compare January 9, 2025 09:49
…to data pointers. Make conversions to StrRange implicit to aid transition.
@Ivorforce Ivorforce force-pushed the string-to-pointer-conversion branch from e2f7611 to edb8339 Compare January 9, 2025 09:51
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Jan 9, 2025

I have reset two calls I had changed from latin1 parsing to utf8 parsing. It was incorrect to change them; they should be assessed on a case by case basis. If I haven't missed anything all calls I changed to call get_data() explicitly will now behave the exact same as before.

I assume this is superseded by #101304, which include all changes from this PR.

Yes! Although I would be happier if this PR was merged / approved first because it has some implications that are not discussed in the #101304 separately.

@akien-mga
Copy link
Member

I assume this is superseded by #101304, which include all changes from this PR.

The commit here is a pre-requisite in #101304, but it would be best to approve and merge this first, and then rebase #101304, to keep concerns separate.

@Repiteo Repiteo removed this from the 4.x milestone Jan 9, 2025
@Repiteo Repiteo added this to the 4.4 milestone Jan 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.

6 participants