Use Span directly when available #9215
Conversation
|
You can test this PR using the following package version. |
src/Avalonia.Base/Media/Color.cs
Outdated
| inString.Substring(0, percentIndex), | ||
| NumberStyles.Number, | ||
| CultureInfo.InvariantCulture, | ||
| var result = inString.Slice(0, percentIndex).TryParseNumberToDouble( |
There was a problem hiding this comment.
I'm not a big fan of the new TryParseNumberToDouble type methods here:
- It assumes invariant culture and number styles (which I guess that's why the name is so long).
- Using the actual double.TryParse() is so much cleaner and more standards-friendly. It's clear culture is being handled correctly.
- The new methods don't actually do anything right unless compiled for .net versions that use spans. In .net standard you are actually going string -> span -> string. This is why I really didn't do this before. With this in mind you can't actually close Add internal Span-helpers to speed up parsing in some code parts #7912 either.
There was a problem hiding this comment.
I would really just add a new version of double.TryParse() that supports span and .net standard 2.0 then use it here. Mentioned: https://github.com/AvaloniaUI/Avalonia/pull/9215/files#r997344774. That makes the code more future-proof, readable and culture-aware IMO.
There was a problem hiding this comment.
About #3, I would actually say that we don't need to overcomplicate task just for netstandard2.0.
Majority of supported platforms will use .NET 6 or higher. Where netstandard2.0 is kept for small percent of .NET Framework apps build with Avalonia (mostly, as an integration with older framework apps) and some exotic Mono devices.
There was a problem hiding this comment.
1-2. This is easier to read and should help avoid culture issues now that new parameters are added for number style and culture. Thanks.
3. I don't know the platforms or applications netstandard 2.0 is used for so if you think it's fine I won't disagree.
| return double.TryParse(span.ToString(), NumberStyles.Number, CultureInfo.InvariantCulture, | ||
| out value); | ||
| #else | ||
| return double.TryParse(span, NumberStyles.Number, CultureInfo.InvariantCulture, |
There was a problem hiding this comment.
This is really the correct fix. Honestly, I would copy-paste this method out of newer .net versions and just add it Avalonia. Then we can use it uniformly in new code.
There was a problem hiding this comment.
It would be similar in concept to MathUtilities.Clamp() which just re-implements exactly what .net does in newer versions. Then when we eventually move over, the code can quickly be updated by just using the new method. No need to re-work the code structure or properties.
There was a problem hiding this comment.
I don't know if copying the newer span implementations would be necessary here. The issue only required using the provided span overloads from Net 6/ Net standard 2.1 if available, and fallback to plain string methods if not. There's nothing to fallback unto with the MathUtilities helpers
There was a problem hiding this comment.
Yes, the point was add methods to fallback to in .net standard 2.0. MathUtilities was just an example. Different direction is fine though since it's already merged.
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
What does the pull request do?
Adds implicit string operator for SpanCompat, and helpers for parsing span to value types to avoid string allocation.
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Addresses #7912