Skip to content

Commit

Permalink
[core] use StringComparison.Ordinal everywhere (#4988)
Browse files Browse the repository at this point in the history
Context: https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1307
Context: https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1309
Context: dotnet/runtime#43956

I was reviewing `dotnet trace` output of the .NET Podcast app:

    6.32ms Microsoft.Maui.Controls!Microsoft.Maui.Controls.ShellNavigationManager.GetNavigationState
    3.82ms Microsoft.Maui.Controls!Microsoft.Maui.Controls.ShellUriHandler.FormatUri

The bulk of this time is spent in `string.StartsWith()`, doing a
culture-aware string comparison?

    3.82ms System.Private.CoreLib!System.String.StartsWith
    2.57ms System.Private.CoreLib!System.Globalization.CultureInfo.get_CurrentCulture

This looks to be showing the cost of the 1st culture-aware comparision
plus any time making these slower string comparisons. It would be
ideal if project templates did not even hit
`CultureInfo.get_CurrentCulture`.

To solve this, let's add to the `.editorconfig` file:

    dotnet_diagnostic.CA1307.severity = error
    dotnet_diagnostic.CA1309.severity = error

And then fix all the places errors appear.

There are some complications in projects that use `netstandard2.0`:

1. For `Contains()` use `IndexOf()` instead.

2. Use `#if NETSTANDARD2_0` for `GetHashCode()` or `Replace()`. Use the `char` overload
of `string.Replace()` where possible.

3. Generally, `InvariantCulture` should not be used.

After these changes, the `FormatUri` method disappears completely from
the trace, and we instead get:

    2.88ms Microsoft.Maui.Controls!Microsoft.Maui.Controls.ShellNavigationManager.GetNavigationState

I suspect this saves ~3.44ms for any MAUI app startup, and a small
amount more depending on the number of string comparisions happening.
  • Loading branch information
jonathanpeppers committed Mar 1, 2022
1 parent 439b52c commit c40c6e7
Show file tree
Hide file tree
Showing 88 changed files with 248 additions and 182 deletions.
4 changes: 4 additions & 0 deletions .editorconfig
Expand Up @@ -23,6 +23,10 @@ indent_style = tab
# https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1416
dotnet_diagnostic.CA1416.severity = none

# Code analyzers
dotnet_diagnostic.CA1307.severity = error
dotnet_diagnostic.CA1309.severity = error

# Modifier preferences
dotnet_style_require_accessibility_modifiers = never:suggestion

Expand Down
2 changes: 1 addition & 1 deletion src/BlazorWebView/src/Maui/Windows/WinUIWebViewManager.cs
Expand Up @@ -91,7 +91,7 @@ protected override async Task HandleWebResourceRequest(CoreWebView2WebResourceRe
{
relativePath = _hostPageRelativePath;
}
relativePath = Path.Combine(_contentRootDir, relativePath.Replace("/", "\\"));
relativePath = Path.Combine(_contentRootDir, relativePath.Replace('/', '\\'));

var winUIItem = await Package.Current.InstalledLocation.TryGetItemAsync(relativePath);
if (winUIItem != null)
Expand Down
4 changes: 3 additions & 1 deletion src/BlazorWebView/src/SharedSource/QueryStringHelper.cs
Expand Up @@ -3,6 +3,8 @@

#nullable enable

using System;

namespace Microsoft.AspNetCore.Components.WebView
{
internal static class QueryStringHelper
Expand All @@ -13,7 +15,7 @@ public static string RemovePossibleQueryString(string? url)
{
return string.Empty;
}
var indexOfQueryString = url.IndexOf('?');
var indexOfQueryString = url.IndexOf('?', StringComparison.Ordinal);
return (indexOfQueryString == -1)
? url
: url.Substring(0, indexOfQueryString);
Expand Down
@@ -1,3 +1,4 @@
using System;
using Microsoft.Maui.Controls.Compatibility.ControlGallery;

namespace Microsoft.Maui.Controls.Compatibility.ControlGallery.Android
Expand All @@ -8,7 +9,7 @@ public bool IsOnTestCloud()
{
var isInTestCloud = System.Environment.GetEnvironmentVariable("XAMARIN_TEST_CLOUD");

return isInTestCloud != null && isInTestCloud.Equals("1");
return isInTestCloud != null && isInTestCloud.Equals("1", StringComparison.Ordinal);
}

public string GetTestCloudDeviceName()
Expand Down
Expand Up @@ -40,7 +40,7 @@ public DemoFilteredItemSource(int count = 50, Func<string, CollectionViewGallery
private bool ItemMatches(string filter, CollectionViewGalleryTestItem item)
{
filter = filter ?? "";
return item.Caption.ToLower().Contains(filter?.ToLower());
return item.Caption.IndexOf(filter, StringComparison.OrdinalIgnoreCase) != -1;
}

public void FilterItems(string filter)
Expand Down
Expand Up @@ -15,7 +15,7 @@ public static Font ParseUIFont(string font)

// Logger.LogLine ("TEST PARSING");

if (font.Contains("font-weight: bold;"))
if (font.IndexOf("font-weight: bold;", StringComparison.Ordinal) != -1)
{
// Logger.LogLine ("Found Bold");
fontAttrs = FontAttributes.Bold;
Expand Down
2 changes: 1 addition & 1 deletion src/Compatibility/ControlGallery/src/WinUI/WinUIStartup.cs
Expand Up @@ -26,7 +26,7 @@ public static MauiApp CreateMauiApp()
.AddWindows(windows => windows
.OnLaunching((_, e) =>
{
if (!string.IsNullOrWhiteSpace(e.Arguments) && e.Arguments.Contains("RunningAsUITests"))
if (!string.IsNullOrWhiteSpace(e.Arguments) && e.Arguments.Contains("RunningAsUITests", StringComparison.Ordinal))
{
App.RunningAsUITests = true;
ControlGallery.App.PreloadTestCasesIssuesList = false;
Expand Down
2 changes: 1 addition & 1 deletion src/Compatibility/ControlGallery/src/iOS/AppDelegate.cs
Expand Up @@ -89,7 +89,7 @@ public bool IsOnTestCloud()
{
var isInTestCloud = Environment.GetEnvironmentVariable("XAMARIN_TEST_CLOUD");

return isInTestCloud != null && isInTestCloud.Equals("1");
return isInTestCloud != null && isInTestCloud.Equals("1", StringComparison.Ordinal);
}

public string GetTestCloudDeviceName()
Expand Down
22 changes: 11 additions & 11 deletions src/Compatibility/Core/src/Android/BorderBackgroundManager.cs
Expand Up @@ -219,17 +219,17 @@ void BorderElementPropertyChanged(object sender, PropertyChangedEventArgs e)
return;
}

if (e.PropertyName.Equals(Button.BorderColorProperty.PropertyName) ||
e.PropertyName.Equals(Button.BorderWidthProperty.PropertyName) ||
e.PropertyName.Equals(Button.CornerRadiusProperty.PropertyName) ||
e.PropertyName.Equals(VisualElement.BackgroundColorProperty.PropertyName) ||
e.PropertyName.Equals(VisualElement.BackgroundProperty.PropertyName) ||
e.PropertyName.Equals(Specifics.Button.UseDefaultPaddingProperty.PropertyName) ||
e.PropertyName.Equals(Specifics.Button.UseDefaultShadowProperty.PropertyName) ||
e.PropertyName.Equals(Specifics.ImageButton.IsShadowEnabledProperty.PropertyName) ||
e.PropertyName.Equals(Specifics.ImageButton.ShadowColorProperty.PropertyName) ||
e.PropertyName.Equals(Specifics.ImageButton.ShadowOffsetProperty.PropertyName) ||
e.PropertyName.Equals(Specifics.ImageButton.ShadowRadiusProperty.PropertyName))
if (e.PropertyName.Equals(Button.BorderColorProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(Button.BorderWidthProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(Button.CornerRadiusProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(VisualElement.BackgroundColorProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(VisualElement.BackgroundProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(Specifics.Button.UseDefaultPaddingProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(Specifics.Button.UseDefaultShadowProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(Specifics.ImageButton.IsShadowEnabledProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(Specifics.ImageButton.ShadowColorProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(Specifics.ImageButton.ShadowOffsetProperty.PropertyName, StringComparison.Ordinal) ||
e.PropertyName.Equals(Specifics.ImageButton.ShadowRadiusProperty.PropertyName, StringComparison.Ordinal))
{
Reset();
UpdateDrawable();
Expand Down
Expand Up @@ -160,7 +160,7 @@ void SetDate(DateTime date)
{
EditText.Text = date.ToShortDateString();
}
else if (Element.Format.Contains('/'))
else if (Element.Format.Contains('/', StringComparison.Ordinal))
{
EditText.Text = date.ToString(Element.Format, CultureInfo.InvariantCulture);
}
Expand Down
Expand Up @@ -20,7 +20,7 @@ public abstract class TimePickerRendererBase<TControl> : ViewRenderer<TimePicker
[PortHandler]
bool Is24HourView
{
get => (DateFormat.Is24HourFormat(Context) && Element.Format == (string)TimePicker.FormatProperty.DefaultValue) || Element.Format?.Contains('H') == true;
get => (DateFormat.Is24HourFormat(Context) && Element.Format == (string)TimePicker.FormatProperty.DefaultValue) || Element.Format?.Contains('H', StringComparison.Ordinal) == true;
}

public TimePickerRendererBase(Context context) : base(context)
Expand Down
2 changes: 1 addition & 1 deletion src/Compatibility/Core/src/Android/ResourceManager.cs
Expand Up @@ -405,7 +405,7 @@ static int IdFromTitle(string title, [DynamicallyAccessedMembers(DynamicallyAcce

// When searching by reflection you would use a "_" instead of a "."
// So this accounts for cases where users were searching with an "_"
if ((id = SearchByIdentifier(name.Replace("_", "."), defType, resource, packageName)) > 0)
if ((id = SearchByIdentifier(name.Replace("_", ".", StringComparison.Ordinal), defType, resource, packageName)) > 0)
return id;

int SearchByIdentifier(string n, string d, Resources r, string p)
Expand Down
8 changes: 4 additions & 4 deletions src/Compatibility/Core/src/Windows/DatePickerRenderer.cs
Expand Up @@ -143,7 +143,7 @@ void OnControlDateChanged(object sender, DatePickerValueChangedEventArgs e)

bool CheckDateFormat()
{
return String.IsNullOrWhiteSpace(Element.Format) || Element.Format.Equals("d");
return String.IsNullOrWhiteSpace(Element.Format) || Element.Format.Equals("d", StringComparison.Ordinal);
}

[PortHandler]
Expand All @@ -165,7 +165,7 @@ void UpdateMonth()
{
Control.MonthFormat = "month";
}
else if (Element.Format.Equals("D"))
else if (Element.Format.Equals("D", StringComparison.Ordinal))
{
Control.MonthFormat = "month.full";
}
Expand All @@ -191,7 +191,7 @@ void UpdateDay()
{
Control.DayFormat = "day";
}
else if (Element.Format.Equals("D"))
else if (Element.Format.Equals("D", StringComparison.Ordinal))
{
Control.DayFormat = "dayofweek.full";
}
Expand All @@ -217,7 +217,7 @@ void UpdateYear()
{
Control.YearFormat = "year";
}
else if (Element.Format.Equals("D"))
else if (Element.Format.Equals("D", StringComparison.Ordinal))
{
Control.YearFormat = "year.full";
}
Expand Down
Expand Up @@ -70,7 +70,7 @@ void UpdateImageDirectory(FileImageSource fileSource)

var directory = IOPath.GetDirectoryName(filePath);

if (string.IsNullOrEmpty(directory) || !IOPath.GetFullPath(directory).Equals(IOPath.GetFullPath(imageDirectory)))
if (string.IsNullOrEmpty(directory) || !IOPath.GetFullPath(directory).Equals(IOPath.GetFullPath(imageDirectory), StringComparison.Ordinal))
{
filePath = IOPath.Combine(imageDirectory, filePath);
fileSource.File = filePath;
Expand Down
Expand Up @@ -124,7 +124,7 @@ string GetFontSource(FontImageSource fontImageSource)

foreach(var family in allFamilies)
{
if(family.Contains(source))
if(family.Contains(source, StringComparison.Ordinal))
{
fontSource = family;
break;
Expand Down
2 changes: 1 addition & 1 deletion src/Compatibility/Core/src/Windows/TimePickerRenderer.cs
Expand Up @@ -168,7 +168,7 @@ void UpdateFont()
void UpdateTime()
{
Control.Time = Element.Time;
if (Element.Format?.Contains('H') == true)
if (Element.Format?.Contains('H', StringComparison.Ordinal) == true)
{
Control.ClockIdentifier = "24HourClock";
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compatibility/Core/src/Windows/WebViewRenderer.cs
Expand Up @@ -52,7 +52,7 @@ public void LoadHtml(string html, string baseUrl)
_internalWebView.NavigationCompleted += async (sender, args) =>
{
// Generate a version of the <base> script with the correct <base> tag
var script = BaseInsertionScript.Replace("baseTag", baseTag);
var script = BaseInsertionScript.Replace("baseTag", baseTag, StringComparison.Ordinal);
// Run it and retrieve the updated HTML from our WebView
await sender.ExecuteScriptAsync(script);
Expand Down
Expand Up @@ -22,7 +22,7 @@ public NativeViewPropertyListener(string targetProperty)

public override void ObserveValue(NSString keyPath, NSObject ofObject, NSDictionary change, IntPtr context)
{
if (keyPath.ToString().Equals(TargetProperty, StringComparison.InvariantCultureIgnoreCase))
if (keyPath.ToString().Equals(TargetProperty, StringComparison.OrdinalIgnoreCase))
PropertyChanged?.Invoke(ofObject, new PropertyChangedEventArgs(TargetProperty));
else
base.ObserveValue(keyPath, ofObject, change, context);
Expand Down
Expand Up @@ -176,12 +176,12 @@ void UpdateDateFromModel(bool animate)
_picker.SetDate(Element.Date.ToNSDate(), animate);

// Can't use Element.Format because it won't display the correct format if the region and language are set differently
if (string.IsNullOrWhiteSpace(Element.Format) || Element.Format.Equals("d") || Element.Format.Equals("D"))
if (string.IsNullOrWhiteSpace(Element.Format) || Element.Format.Equals("d", StringComparison.OrdinalIgnoreCase))
{
NSDateFormatter dateFormatter = new NSDateFormatter();
dateFormatter.TimeZone = NSTimeZone.FromGMT(0);

if (Element.Format?.Equals("D") == true)
if (Element.Format?.Equals("D", StringComparison.Ordinal) == true)
{
dateFormatter.DateStyle = NSDateFormatterStyle.Long;
var strDate = dateFormatter.StringFor(_picker.Date);
Expand All @@ -194,7 +194,7 @@ void UpdateDateFromModel(bool animate)
Control.Text = strDate;
}
}
else if (Element.Format.Contains("/"))
else if (Element.Format.Contains('/', StringComparison.Ordinal))
{
Control.Text = Element.Date.ToString(Element.Format, CultureInfo.InvariantCulture);
}
Expand Down
Expand Up @@ -233,13 +233,13 @@ void UpdateTime()
Control.Text = DateTime.Today.Add(Element.Time).ToString(Element.Format, cultureInfos);
}

if (Element.Format?.Contains('H') == true)
if (Element.Format?.Contains('H', StringComparison.Ordinal) == true)
{
var ci = new System.Globalization.CultureInfo("de-DE");
NSLocale locale = new NSLocale(ci.TwoLetterISOLanguageName);
_picker.Locale = locale;
}
else if (Element.Format?.Contains('h') == true)
else if (Element.Format?.Contains('h', StringComparison.Ordinal) == true)
{
var ci = new System.Globalization.CultureInfo("en-US");
NSLocale locale = new NSLocale(ci.TwoLetterISOLanguageName);
Expand Down
4 changes: 2 additions & 2 deletions src/Compatibility/Core/src/iOS/Renderers/WkWebViewRenderer.cs
Expand Up @@ -365,7 +365,7 @@ async Task<List<NSHttpCookie>> GetCookiesFromNativeStore(string url)
{
// we don't care that much about this being accurate
// the cookie container will split the cookies up more correctly
if (!cookie.Domain.Contains(domain) && !domain.Contains(cookie.Domain))
if (!cookie.Domain.Contains(domain, StringComparison.Ordinal) && !domain.Contains(cookie.Domain, StringComparison.Ordinal))
continue;

existingCookies.Add(cookie);
Expand Down Expand Up @@ -563,7 +563,7 @@ async Task DeleteCookies(List<NSHttpCookie> cookies)
foreach (var deleteme in cookies)
{
if (record.DisplayName.Contains(deleteme.Domain) || deleteme.Domain.Contains(record.DisplayName))
if (record.DisplayName.Contains(deleteme.Domain, StringComparison.Ordinal) || deleteme.Domain.Contains(record.DisplayName, StringComparison.Ordinal))
{
WKWebsiteDataStore.DefaultDataStore.RemoveDataOfTypes(record.DataTypes,
new[] { record }, () => { });
Expand Down
11 changes: 10 additions & 1 deletion src/Controls/Maps/src/Pin.cs
Expand Up @@ -70,10 +70,17 @@ public override int GetHashCode()
{
unchecked
{
#if NETSTANDARD2_0
int hashCode = Label?.GetHashCode() ?? 0;
hashCode = (hashCode * 397) ^ Position.GetHashCode();
hashCode = (hashCode * 397) ^ (int)Type;
hashCode = (hashCode * 397) ^ (Address?.GetHashCode() ?? 0);
#else
int hashCode = Label?.GetHashCode(StringComparison.Ordinal) ?? 0;
hashCode = (hashCode * 397) ^ Position.GetHashCode();
hashCode = (hashCode * 397) ^ (int)Type;
hashCode = (hashCode * 397) ^ (Address?.GetHashCode(StringComparison.Ordinal) ?? 0);
#endif
return hashCode;
}
}
Expand Down Expand Up @@ -106,7 +113,9 @@ public bool SendInfoWindowClick()

bool Equals(Pin other)
{
return string.Equals(Label, other.Label) && Equals(Position, other.Position) && Type == other.Type && string.Equals(Address, other.Address);
return string.Equals(Label, other.Label, StringComparison.Ordinal) &&
Equals(Position, other.Position) && Type == other.Type &&
string.Equals(Address, other.Address, StringComparison.Ordinal);
}
}
}
Expand Up @@ -36,7 +36,7 @@ private bool ItemMatches(string filter, CollectionViewGalleryTestItem item)
return true;
}

return item.Date.DayOfWeek.ToString().ToLower().Contains(filter.ToLower());
return item.Date.DayOfWeek.ToString().Contains(filter, StringComparison.OrdinalIgnoreCase);
}
}

Expand Down
Expand Up @@ -40,7 +40,7 @@ public DemoFilteredItemSource(int count = 50, Func<string, CollectionViewGallery
private bool ItemMatches(string filter, CollectionViewGalleryTestItem item)
{
filter = filter ?? "";
return item.Caption.ToLower().Contains(filter?.ToLower());
return item.Caption.Contains(filter, StringComparison.OrdinalIgnoreCase);
}

public void FilterItems(string filter)
Expand Down
10 changes: 9 additions & 1 deletion src/Controls/src/Core/Accelerator.cs
Expand Up @@ -49,7 +49,11 @@ public static Accelerator FromString(string text)
case "fn":
case "win":
modifiers.Add(modiferMaskLower);
#if NETSTANDARD2_0
text = text.Replace(modifierMask, "");
#else
text = text.Replace(modifierMask, "", StringComparison.Ordinal);
#endif
break;
}
}
Expand Down Expand Up @@ -89,7 +93,11 @@ bool Equals(Accelerator other)
/// <include file="../../docs/Microsoft.Maui.Controls/Accelerator.xml" path="//Member[@MemberName='GetHashCode']/Docs" />
public override int GetHashCode()
{
return ToString().GetHashCode();
#if NETSTANDARD2_0
return _text.GetHashCode();
#else
return _text.GetHashCode(StringComparison.Ordinal);
#endif
}

public static implicit operator Accelerator(string accelerator)
Expand Down
11 changes: 9 additions & 2 deletions src/Controls/src/Core/AnimatableKey.cs
Expand Up @@ -47,18 +47,25 @@ public override int GetHashCode()
unchecked
{
IAnimatable target;
#if NETSTANDARD2_0
if (!Animatable.TryGetTarget(out target))
{
return Handle?.GetHashCode() ?? 0;
}

return ((target?.GetHashCode() ?? 0) * 397) ^ (Handle?.GetHashCode() ?? 0);
#else
if (!Animatable.TryGetTarget(out target))
{
return Handle?.GetHashCode(StringComparison.Ordinal) ?? 0;
}
return ((target?.GetHashCode() ?? 0) * 397) ^ (Handle?.GetHashCode(StringComparison.Ordinal) ?? 0);
#endif
}
}

protected bool Equals(AnimatableKey other)
{
if (!string.Equals(Handle, other.Handle))
if (!string.Equals(Handle, other.Handle, StringComparison.Ordinal))
{
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Controls/src/Core/BindablePropertyConverter.cs
Expand Up @@ -81,7 +81,7 @@ public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo c

if (string.IsNullOrWhiteSpace(strValue))
return null;
if (strValue.Contains(":"))
if (strValue.IndexOf(":", StringComparison.Ordinal) != -1)
{
Application.Current?.FindMauiContext()?.CreateLogger<BindablePropertyConverter>()?.LogWarning("Can't resolve properties with xml namespace prefix.");
return null;
Expand Down

0 comments on commit c40c6e7

Please sign in to comment.