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

Throw runtime error for range with last == numeric_limits<T>::max() #1181

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

sookach
Copy link
Contributor

@sookach sookach commented Jul 25, 2024

Addresses #1180. The short of the issue is that if we create a numeric range with last == numeric_limits::max(), the ++last in the constructor causes overflow, which is not great to say the least. I can't think of a clean way to address this issue since a core aspect of c++ iterators is their asymmetry, so I figured it's better just to report an error if we notice a user trying to create an erroneous range. Open to suggestions.

@gregmarr
Copy link
Contributor

Seems reasonable. A compile time check would be good for when the bounds are a compile time constant.

Another useful check would be first <= last.

@hsutter
Copy link
Owner

hsutter commented Jul 25, 2024

Thanks! I just saw this after I pushed my fix for this. Our minds think alike -- what you wrote is pretty much exactly one of the options I wrote and considered, before backing it out when I had the idea for the other approach.

@hsutter hsutter closed this Jul 25, 2024
@hsutter
Copy link
Owner

hsutter commented Jul 25, 2024

Closing this as the other commit has fixed the issue. Thanks again!

@hsutter hsutter reopened this Jul 25, 2024
@hsutter
Copy link
Owner

hsutter commented Jul 25, 2024

Wait, on second thought: This could still be complementary, because I still don't support a range whose end value is numeric_limits< [size_t | ptrdiff_t ] >::max(). I think not supporting that case is fine... those are typically used as invalid/sentinel values, not valid program values that you'd want to include in a closed range, I think. But we could throw if they're encountered...

include/cpp2util.h Outdated Show resolved Hide resolved
include/cpp2util.h Outdated Show resolved Hide resolved
@sookach
Copy link
Contributor Author

sookach commented Jul 25, 2024

Seems reasonable. A compile time check would be good for when the bounds are a compile time constant.

Another useful check would be first <= last.

That's a good idea. I think reverse ranges (i.e. decrementing) might be a useful feature to add to the language, so maybe if @hsutter agrees I can go ahead and start working on implementing those, at which point this check would no longer be necessary.

@gregmarr
Copy link
Contributor

gregmarr commented Jul 25, 2024

Another complementary feature to decrementing ranges is stepped ranges, such as 1...9...2 that generates the odd single-digit numbers. If you do that, you do have to watch out for somewhat "degenerate" cases where the end value is not generated by the step, such as 1...10...2, so you have to do less than or greater than comparisons to find the end condition.

@sookach
Copy link
Contributor Author

sookach commented Jul 25, 2024

Another complementary feature to decrementing ranges is stepped ranges, such as 1...9...2 that generates the odd single-digit numbers. If you do that, you do have to watch out for somewhat "degenerate" cases where the end value is not generated by the step, such as 1...10...2, so you have to do less than or greater than comparisons to find the end condition.

Awesome, thanks for the note.

@hsutter
Copy link
Owner

hsutter commented Jul 25, 2024

I think reverse ranges (i.e. decrementing) might be a useful feature to add to the language

I'd like to keep the language feature simple and make sure it works well with C++20 ranges and views. I was tempted to add more, including a step option and a + to concatenate ranges, and I stopped myself... I think simpler is better, and it made me check that this works with C++20 ranges and views (0 ... 100 .views::take(5) works fine now). I see we have std::ranges::reverse but these ranges don't work with it yet... I'll take a look at what concepts constraints are failing...

For example, in Rust reverse and step are library implementations I think:

for i in (0..100).rev().step_by(5) {
    println!("{}", i);
}

Seems pretty usable? Would a language feature that complicates the grammar do a lot better?

I think the main benefit comes from expressing common things like 0..=100 and first...last better than std::views::iota(0,101). As long as the manipulation functions work that should be fine? If the namespaces names are really ugly, we could add common ones right in cpp2util.h beside class range that UFCS will be able to pick up.

@gregmarr
Copy link
Contributor

I did some more research on ranges and stepping, and found that there are definitely a lot of varieties out there, so maybe just having the basic syntax for now is best, especially if you can do something like (1..=100).stride(2).

@hsutter hsutter merged commit 8060dc4 into hsutter:main Jul 25, 2024
29 checks passed
@hsutter
Copy link
Owner

hsutter commented Jul 25, 2024

Thanks!

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.

3 participants