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.

Sign up for GitHub

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

Jump to bottom

Expand System.Runtime.InteropServices.NFloat to support the APIs required by Xamarin #64234

Merged
merged 7 commits into from
Jan 28, 2022
Merged

Expand System.Runtime.InteropServices.NFloat to support the APIs required by Xamarin #64234

merged 7 commits into from
Jan 28, 2022

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Jan 24, 2022 •
edited
Loading

This resolves #63801.

Tests still need to be added.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned tannergooding Jan 24, 2022
@tannergooding
Copy link
Member Author

CC. @rolfbjarne, @marek-safar, @jeffhandley

@tannergooding tannergooding mentioned this pull request Jan 24, 2022
Closed
rolfbjarne
rolfbjarne requested changes Jan 25, 2022
View reviewed changes
Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

Looks like two operators are reversed?

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NFloat.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NFloat.cs Outdated Show resolved Hide resolved
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 25, 2022
@tannergooding @rolfbjarne
Apply suggestions from code review
5e25e30 
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 25, 2022
@tannergooding tannergooding mentioned this pull request Jan 27, 2022
Closed
@tannergooding tannergooding marked this pull request as ready for review January 27, 2022 19:21
stephentoub
stephentoub reviewed Jan 28, 2022
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NFloat.cs
[NonVersionable]
public static bool operator !=(NFloat left, NFloat right) => left._value != right._value;

/// <summary>Compares two values to determine which is less.</summary>
Copy link
Member

@stephentoub stephentoub Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: This description seems a bit misleading, as it tells you whether left is less than right but not whether right is strictly less then left.

Consider adopting the existing doc terminology that float/double use, e.g. "Returns a value that indicates whether a specified Double value is less than or equal to another specified Double value."

That goes for all these doc comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Consider adopting the existing doc terminology

I always forget that we have APIs exposed here that are functionally unavailable and will never be called 👍

I'll update this in a follow up PR as I need to also update IComparisonOperators and a few other places accordingly.

stephentoub
stephentoub approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Gave it a quick skim and generally LGTM.

@tannergooding tannergooding mentioned this pull request Jan 28, 2022
Closed
@tannergooding tannergooding merged commit f2538c6 into dotnet:main Jan 28, 2022
@tannergooding
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1774427144

@github-actions github-actions bot mentioned this pull request Jan 31, 2022
Merged
@uweigand uweigand mentioned this pull request Jan 31, 2022
Closed
@uweigand uweigand mentioned this pull request Feb 11, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Mar 3, 2022
@tannergooding tannergooding deleted the fix-63801 branch November 11, 2022 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rolfbjarne rolfbjarne rolfbjarne approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees

@tannergooding tannergooding

Labels
area-System.Runtime.InteropServices new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend System.Runtime.InteropServices.NFloat so it can replace the Xamarin in-box nfloat for .NET 7
3 participants
@tannergooding @rolfbjarne @stephentoub