Exclude string.ToLowerInvariant() in GetEnvironmentVariableAsBool() #14323
Conversation
|
@iSazonov I doubt the usefulness of this change. The most common scenario would be that the Your changes here has zero impact to the most common scenario. It improves a less common scenario with the cost of decreased readability, so I'm not convinced we want it. @JamesWTruher, can you please review and share your thoughts? |
|
@daxian-dbw |
|
@JamesWTruher Additional context info: |
|
I'm somewhat in agreement with @daxian-dbw here. I don't see any actual measurement of performance change. If this is about improving performance, I would love to see what this actually gets us. |
|
I would like to avoid early loading of ICU. This is how it might look: |
There was a problem hiding this comment.
@iSazonov I understand it's desired to avoid ToLowerInvariant. I think the concern is, the change improves a less common code path with the cost in readability. I will talk with @JamesWTruher more about it.
| } | ||
|
|
||
| switch (str.ToLowerInvariant()) | ||
| var boolStr = str.AsSpan().Trim(); |
There was a problem hiding this comment.
This is a behavior change, that allows values like " yes " to work, which was discarded before.
There was a problem hiding this comment.
This follow the intention to support many acceptable values.
But I wonder why we need this at all. We could support only "0" and "1" values or better to check only presents the variable as a flag.
If we simplify the rules we simplify the code.
There was a problem hiding this comment.
Don't change behavior when doing perf optimization. Also, we cannot do breaking changes.
There was a problem hiding this comment.
I removed the Trim().
Code below comes from .Net https://source.dot.net/#System.Private.CoreLib/Boolean.cs,198ff42f14d8c64b
I could make the method internal and add xUnit test for the method.
There was a problem hiding this comment.
I removed the Trim().
@iSazonov Can you push your latest change?
There was a problem hiding this comment.
Oh, sorry - forgot to push the commit.
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
There was a problem hiding this comment.
@iSazonov Sorry for being sluggish. We can merge this one after you address the one more comment I left.
Sometimes holidays take more energy than work 😆 |
|
🎉 Handy links: |
PR Summary
Exclude string.ToLowerInvariant() in GetEnvironmentVariableAsBool() to avoid early ICU initialization.
PR Context
Related #14268.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.