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

Use string.IsNullOrEmpty and ArgumentException.ThrowIfNullOrEmpty in many more places #85858

Merged
merged 1 commit into from
May 9, 2023

Conversation

stephentoub
Copy link
Member

We have an analyzer for ThrowIfNullOrEmpty; I'll separately look into why it wasn't firing.

We should consider an analyzer for string.IsNullOrEmpty. There's no perf benefit today, other than smaller IL, but the resulting code is easier to read IMHO, and there can be a benefit in the few cases where a complicated expression is duplicated and the JIT can't CSE the whole thing.

@ghost
Copy link

ghost commented May 6, 2023

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

We have an analyzer for ThrowIfNullOrEmpty; I'll separately look into why it wasn't firing.

We should consider an analyzer for string.IsNullOrEmpty. There's no perf benefit today, other than smaller IL, but the resulting code is easier to read IMHO, and there can be a benefit in the few cases where a complicated expression is duplicated and the JIT can't CSE the whole thing.

Author: stephentoub
Assignees: stephentoub
Labels:

area-Meta

Milestone: -

@am11
Copy link
Member

am11 commented May 10, 2023

Unfortunately, forward substitution still does not handle IsNullOrEmpty() vs. ? true : false, as latter is better in some cases. i.e. in https://godbolt.org/z/rc4Kx56aq M2 is worse than M3 and M1 has the best code.

@stephentoub
Copy link
Member Author

stephentoub commented May 10, 2023

Unfortunately, forward substitution still does not handle IsNullOrEmpty() vs. ? true : false, as latter is better in some cases. i.e. in https://godbolt.org/z/rc4Kx56aq M2 is worse than M3 and M1 has the best code.

Is this just a general comment or is this relevant to the changes made in this PR? This PR didn't remove any uses of ? true : false, and in your godbolt example the differences between s is null || s.Length == 0 vs string.IsNullOrEmpty() are purely about the number of returns as used by the C# compiler:
SharpLab
and even that difference evaporates if you do anything other than directly return the result of the operation, which also doesn't happen in this PR.

@MichalPetryka
Copy link
Contributor

are purely about the number of returns as used by the C# compiler

In this case the number of returns matches.

@stephentoub
Copy link
Member Author

stephentoub commented May 10, 2023

In this case the number of returns matches.

Yes, but again, where in this PR is there code that does that?

(And even if there is an example or two, it doesn't matter in practice... and even if it did matter in practice, that's an issue to be addressed in the JIT, not by changing the code a developer would write to avoid idiomatic patterns.)

@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants