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

Generic Enumerable.Range method #106925

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

AlexRadch
Copy link
Contributor

Close #97689

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, 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.

1 similar comment
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, 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.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 24, 2024

public static IEnumerable<T> Range<T>(T start, int count) where T : IBinaryInteger<T>
{
if (count < 0)
Copy link
Member

Choose a reason for hiding this comment

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

Curious why this overload is using somewhat different validation flow compared to the existing method? Is it anticipating that T.CreateTruncating might be expensive?

@@ -73,27 +101,33 @@ public override void Dispose()
{
_state = -1; // Don't reset current
}

internal static int StartMaxCount(T start, T max)
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this is effectively trying to measure a distance between start and max? I'm not sure if this is necessarily a well-defined operation for every kind of binary integer type (e.g. Gaussian integers)

cc @tannergooding

};
}

public class RangeBigIntegerTests : RangeIBinaryIntegerTests<BigInteger>
Copy link
Member

Choose a reason for hiding this comment

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

It would be interesting to see a few nonstandard numeric types that stress test the implementation, e.g. a modular arithmetic type. If for example I define a type representing all numbers modulo 7, what should be the behaviour of the new method when I do Range(Modulo7.Zero, 8)?

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

The build errors are caused by an expected source breaking change that this new API is introducing. The impacted call sites should be updated with an explicit int type parameter.

It is expected source breaking change that this new API is introducing.
@stephentoub stephentoub added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Sep 4, 2024
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Marking as changes requested since there are a few unresolved questions on a number of corner cases.

@stephentoub
Copy link
Member

Now that #111685 has been merged, we should augment this PR to add the corresponding generic Range method to AsyncEnumerable as well. In general, we want to keep the APIs consistent across Enumerable and AsyncEnumerable.

Updated usage of Enumerable.Range to specify <int> type
parameter in VirtualDriveHelper.Windows.cs,
ReadOnlySequenceFactory.char.cs, and ArrayTests.cs.
This change improves code clarity and enhances type safety
in drive letter generation and ASCII range handling,
as well as strengthens assertions in unit tests.
Introduces a new method `Range<T>(T start, int count)` in the `AsyncEnumerable` class for generating sequences of integral numbers for types implementing the `IBinaryInteger<T>` interface. This method is conditionally compiled for .NET 7.0 or greater and includes validation for the `count` parameter.

Updates `RangeTests` to cover various input scenarios for the new method, including edge cases for different integral types like `byte` and `long`. Additionally, enhances documentation for the existing `Range(int start, int count)` method.
@AlexRadch
Copy link
Contributor Author

Now that #111685 has been merged, we should augment this PR to add the corresponding generic Range method to AsyncEnumerable as well. In general, we want to keep the APIs consistent across Enumerable and AsyncEnumerable.

@stephentoub Done!

for (int i = 0; i < count; i++, start++)
{
yield return start;
}
Copy link
Member

Choose a reason for hiding this comment

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

@tannergooding, is this the right implementation? The above non-generic does:

for (int i = 0; i < count; i++)
{
    yield return start + i;
}

Is this proposed implementation better or worse than:

for (int i = 0; i < count; i++)
{
    yield return start + T.CreateTruncating(i);
}

? I'm not sure what requirements we place on IBinaryInteger, in terms of guarantees we make about ++ actually incrementing the value (as opposed to, say, a float + 1 which might not actually increment).

if (count < 0)
count = int.CreateTruncating(max) - int.CreateTruncating(start);
return count;
}
Copy link
Member

Choose a reason for hiding this comment

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

@tannergooding, is this how we'd recommend doing overflow checking?

This commit introduces a new implementation of the `Range` method for `IAsyncEnumerable<T>` that supports `IBinaryInteger<T>` types, specifically for .NET Core. The method generates a sequence of integral numbers within a specified range, with input validation and exception handling for out-of-range values. Project files have been updated for conditional compilation targeting .NET Core, and the previous `Range` method for non-`IBinaryInteger<T>` types has been removed. Tests have also been updated to reflect these changes.
- Introduced `System.Numerics` for `BigInteger` usage.
- Added test for `ArgumentOutOfRangeException` with `BigInteger` in `InvalidInputs_Throws`.
- Implemented tests for various `BigInteger` values in `VariousValues_MatchesEnumerable`.
Modified `RangeTests.cs` to test a wider range of `BigInteger` values instead of just `long`. Added a new test case for `AsyncEnumerable.Range<BigInteger>` that checks the behavior when skipping and taking elements from a large range. All tests executed successfully with no errors or failures.
Modified test cases in `InvalidInputs_Throws` in `RangeTests.cs` to validate `ArgumentOutOfRangeException` for negative count values in `AsyncEnumerable.Range` for `byte`, `long`, and `BigInteger` types. All tests passed successfully.
Updated assertions in `Array_ByValueOut` and `Array_ByValueOut_This` methods to cast characters in `testArray` to integers. This ensures proper comparison with the expected output of `Enumerable.Range<int>`, enhancing test accuracy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Linq breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. community-contribution Indicates that the PR has been added by a community member needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Generic Enumerable.Range method
4 participants