Open gafter opened 6 years ago
Consider having a central location that exposes teh right comparers to use, with teh right names to help pick which one you want. For example: StringComparers.FileSystemNameComparer. that would go a long way toward helping people make the right choices.
A StringComparers.IdentifierNameComparer
would also help.
@gafter Il can go through them if you like?
@gafter We should also consider auditing the use of CaseInsensitiveComparison.ToLower
If you need a
StringComparer
that performs case-mapping, you have two choices:StringComparer.OrdinalIgnoreCase
CaseInsensitiveComparison.Comparer
These two case-mapping techniques differ in how they are implemented, and therefore in how they behave.
StringComparer.OrdinalIgnoreCase
uses the invariant mapping to compare two strings mapped to uppercase. This is perfect for file system operations, because that corresponds to the way the Windows file system works. Although the invariant mapping has bugs and is "out of date" with respect to Unicode, it is guaranteed not to change, which is what you want for a file system.CaseInsensitiveComparison.Comparer
uses a recent Unicode version culture-invariant lowercase mapping to compare two strings. This is perfect for language operations, because that corresponds to the way we compare identifiers for VB.The Roslyn code base is inconsistent in how it uses these two. The most common error is using the former when the latter would be more appropriate. Here two examples of probably incorrect uses in the compiler:
Roslyn\src\Compilers\Core\Portable\Collections\IdentifierCollection.cs(26): StringComparer.OrdinalIgnoreCase);
Roslyn\src\Compilers\Core\Portable\Emit\AnonymousTypeKey.cs(43): (IgnoreCase ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal).Equals(Name, other.Name);
We should probably survey the rest of Roslyn for other incorrect uses.