Skip to content

Conversation

@elachlan
Copy link
Contributor

@elachlan elachlan commented Jan 8, 2022

Replacing empty strings with string.Empty

I ran into this as msbuild relies on Microsoft.CodeAnalysis.Collections and the stylecop analyzer highlighted this problem when we turned on the rule SA1122.

The associated msbuild PR is dotnet/msbuild#7239

@ghost ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-IDE labels Jan 8, 2022
@CyrusNajmabadi
Copy link
Member

I don't really get the purpose of this rule.

@elachlan
Copy link
Contributor Author

elachlan commented Jan 9, 2022

I don't really get the purpose of this rule.

As far as I know, it's supposed to reduce string allocations.

@CyrusNajmabadi
Copy link
Member

String literals are already interned. So using "" is not going to be an allocation problem :-)

@elachlan
Copy link
Contributor Author

elachlan commented Jan 9, 2022

Based on my reading, the advantage is in readability and to reduce the chance of a hidden zero width space character.

There was acutally one disadvantage that I saw:
"Another difference is that String.Empty generates larger CIL code"
https://stackoverflow.com/a/17386465/908889

It also isn't a constant:
https://stackoverflow.com/questions/507923/why-isnt-string-empty-a-constant
Which results in some usages being incorrect:
https://stackoverflow.com/a/13703103/908889

I will take this to MSBuild and Stylecop and see if this Analyzer is still needed. Please let me know if any of the above is incorrect and thank you for your help and explanation.

@CyrusNajmabadi
Copy link
Member

Based on my reading, the advantage is in readability and to reduce the chance of a hidden zero width space character.

I personally think the readability is fine :-). The zwsp is interesting. But it hasn't ever been an issue for us, so I don't think it's really needed to change here.

@elachlan
Copy link
Contributor Author

elachlan commented Jan 9, 2022

Agreed. I will suggest msbuild doesn't take on this analyzer. I might look at an analyzer for zwsp to highlight any usages to help detect issues.

Thank you for your help @CyrusNajmabadi it is much appreciated.

@elachlan elachlan closed this Jan 9, 2022
@elachlan elachlan deleted the fix-empty-strings-throwhelper branch January 10, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants