Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FromString supporting all NumberStyles #58

Merged

Conversation

lipchev
Copy link
Contributor

@lipchev lipchev commented May 4, 2024

fixes #56

Sorry again- way too many weird cases for review...

Benchmark results to come in bit,

Feel free to make changes directly to my branch..

@@ -88,26 +89,36 @@ namespace Fractions;
/// <para><c>true</c> if <paramref name="fractionString"/> was well-formed. The parsing result will be written to <paramref name="fraction"/>. </para>
/// <para><c>false</c> if <paramref name="fractionString"/> was invalid.</para>
/// </returns>
public static bool TryParse(string fractionString, NumberStyles numberStyles, IFormatProvider formatProvider,
public static bool TryParse(ReadOnlySpan<char> fractionString, NumberStyles numberStyles, IFormatProvider formatProvider,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that I removed the string version overload in .net8 and renamed the parameter name to match the other one.
Technically this is a breaking change, but only for people who use reflection (or who have recently switched to using the ReadOnlySpan overload and have named the parameter explicitly).

Personally, I think this is very unlikely to break for anyone, while the advantage (IMO) of not having the string overload is that the user would probably notice the little icon indicating an implicit conversion, and consider updating his code with ReadOnlySpan.. 🚀

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think changing the public API really brings much benefit here. Named Arguments are used quite frequently (at least by us).

I'll add the overload again for the git-merge.

@lipchev
Copy link
Contributor Author

lipchev commented May 4, 2024

This is the comparison with master. Note that there are 3 cases for which the allocations (and the mean) in net8 have increased. I have spent a lot of time but could not make them match the base-line. I hope you can spot something that I've missed..

image
image

@lipchev lipchev mentioned this pull request May 5, 2024
@danm-de danm-de merged commit 3355566 into danm-de:master May 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List of possible issues with string parsing in v7
2 participants