Refactoring which numeric interface exposes which methods to match latest API review decisions #69582
Conversation
…test API review decisions
…ng coverage to be more easily seen
|
Note regarding the 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. |
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsThis is broken into a few commits and it may be easiest to review this per commit rather than altogether.
|
…28/UInt128 merge conflicts
dakersnar
left a comment
dakersnar
left a comment
There was a problem hiding this comment.
Looks good to me! Looked less closely at the tests, but they seem robust.
| public static class ExponentialFunctionsHelper<TSelf> | ||
| where TSelf : IExponentialFunctions<TSelf> | ||
| { | ||
| public static TSelf Exp(TSelf x) => TSelf.Exp(x); |
There was a problem hiding this comment.
Who uses these helpers in general? In what context are they useful?
There was a problem hiding this comment.
These are specifically for testing and guarantee that the "static abstracts in interfaces" path is executed, that is the path users will hit by using the generic math feature
| static bool INumberBase<BigInteger>.IsFinite(BigInteger value) => true; | ||
|
|
||
| /// <inheritdoc cref="INumberBase{TSelf}.IsInfinity(TSelf)" /> | ||
| static bool INumberBase<BigInteger>.IsInfinity(BigInteger value) => false; |
There was a problem hiding this comment.
When the rework happens for BigInteger, I wonder if there is any value to introducing the concept of Infinity to handle all numbers above whatever finite limit we set for the type.
There was a problem hiding this comment.
I'd expect that would be overall confusing to users, but its something we can discuss at that time.
| } | ||
|
|
||
| [Fact] | ||
| public static void MaxNumberTest() |
There was a problem hiding this comment.
Only comparing to 1 in each of these tests might not lead to proper coverage, but as long as the 1 input follows a normal code path in each method, this should be fine. For example, if 1 was handled with some edge case in BigInteger.MaxNumber then this would raise an eyebrow, but it doesn't.
There was a problem hiding this comment.
Yeah, there's going to be more tests added overall after the main feature work goes in.
This is more a "minimal subset" to ensure good coverage of the APIs and the normal edge cases.
ghost
locked as resolved and limited conversation to collaborators
stephentoub
added
the
breaking-change
ghost
added
the
needs-breaking-change-doc-created
|
dotnet/docs#41835 was created for the breaking change document |
jeffhandley
removed
the
needs-breaking-change-doc-created
4 participants
This is broken into a few commits and it may be easiest to review this per commit rather than altogether.
INumberBaseas per API review. This includes fixing up the tests to use the right helper type.System.Numerics.ComplexComplexonly implementsINumberBase<T>and so needed more tests overall