-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Remove implicit string operator from Utf8String #122149
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
Conversation
|
This doesn't remove the conversions. I'm actually already working on removing the conversions (pushed out what I have at #122160). This PR will just introduce unnecessary merge conflicts. |
This PR is improving a few places where we were accidentally allocating (calling .ToString() on Utf8String then assigning to Utf8String..) and it isn't regressing anything. It's good to keep allocations explicit to avoid such accidents. |
|
@MichalStrehovsky, merged and updated object writer. |
Thank you! Could you do a pass over these and look at places that do undesirable allocations? The places that currently use the implicit conversion operator are markers for "unreviewed legacy code". They may or may not be causing excessive allocations and conversions. The places are easy to search for. The places that use the string/byte[] constructors right now are "reviewed". They are known not to cause excessive allocations and we're not going to revisit them as part of the UTF-8 effort anymore. This PR switches everything to use the |
692389e to
65bcf82
Compare
65bcf82 to
0fb2c78
Compare
|
@MichalStrehovsky, I’ve reviewed the changes and applied updates in 0fb2c78. If you notice anything that seems off, please let me know. I’ve intentionally left expressions such as Type.Name alone, because these values might include multi-byte characters. |
...ls/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM/ARMReadyToRunHelperNode.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/ba-g I think the globalization issue has a fix in #122480 |
No description provided.