Skip to content

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

Merged
stephentoub merged 1 commit intodotnet:mainfrom
stephentoub:isnullorempty
May 9, 2023
Merged

Use string.IsNullOrEmpty and ArgumentException.ThrowIfNullOrEmpty in many more places#85858
stephentoub merged 1 commit intodotnet:mainfrom
stephentoub:isnullorempty

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented May 6, 2023

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 ghost assigned stephentoub May 6, 2023
@ghost ghost added the area-Meta label May 6, 2023
@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: -

MihaZupan
MihaZupan approved these changes May 6, 2023
View reviewed changes
This was referenced May 9, 2023
Closed
Closed
@runfoapp runfoapp bot mentioned this pull request May 9, 2023
Closed
@stephentoub stephentoub merged commit c0bb818 into dotnet:main May 9, 2023
@stephentoub stephentoub deleted the isnullorempty branch May 9, 2023 22:15
@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 •
edited
Loading

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

MichalPetryka commented May 10, 2023

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 •
edited
Loading

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.

Reviewers

@MihaZupan MihaZupan MihaZupan approved these changes

Assignees

@stephentoub stephentoub

Labels

area-Meta

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stephentoub @am11 @MichalPetryka @MihaZupan