[new RangeInclusive] Convert to half-open in `into_iter` if possible - #159200

scottmcm · 2026-07-13T03:48:03Z

The core goal here is to attempt to remove the penalty between

for x in i..=j { … }

and

for x in i..(j+1) { … }

by changing how range::RangeInclusiveIter works.

In legacy::RangeInclusive, there's no opportunity to run a fixup before the loop, and trying in every iteration of the loop wasn't worth it.

But now that we have the range::RangeInclusive vs range::RangeInclusiveIter split, we can!

The approach here is to, in into_iter, attempt to convert from start..=last to start..(last+1). Regardless whether that worked, we always delegate to a normal Range for iteration, just with a cold check for "actually we need to return one more element".

Basically you can think of this as having start..=last do either (start..(last+1)).chain(None) or (start..last).chain(Some(last)), though stored more efficiently than literally using Chain.

That way, for example, for a slice of non-ZST if you run a for i in 0..=slice.len() loop, it runs exactly the same as if you'd written for i in 0..(slice.len() + 1) (while also still working for slize-of-ZST where slice.len() + 1 might overflow).

r? libs

clarfonthey · 2026-07-14T15:00:22Z

+    // When created from `start..=last`, this range is
+    // - Preferably `start..(last+1)`, so we only need to delegate to the exclusive range
+    // - If necessary (because `last` is a maximal element) `start..last`,
+    //   with the `is_inclusive` field set to `true`
+    range: legacy::Range<A>,
+    // Preferably this is `false`, denoting that we successfully converted the inclusive
+    // range into an exclusive range, and thus have no need for extra handling.
+    // If this is true, however, that means that we must return one final item
+    // after iterating it as an exclusive range.
+    // This must only be true if the iterator is non-empty, implying
+    // `range.start <= range.end`. (If the original inclusive range is empty because
+    // `!(start <= last)`, it's stored as the empty exclusive range `start..last` )
+    is_inclusive: bool,


Since we can and do generate private rustdocs, these should probably just be doc comments.

View changes since the review

clarfonthey · 2026-07-14T15:02:22Z

+            exclusive = &start..&end;
+            &exclusive
+        };
+        fmt::Formatter::debug_tuple_field1_finish(f, "RangeInclusiveIter", field)


Is there any reason this isn't:

Suggested change

fmt::Formatter::debug_tuple_field1_finish(f, "RangeInclusiveIter", field)

f.debug_tuple_field1_finish("RangeInclusiveIter", field)

View changes since the review

I forgot the name of the method so copied from a derive :P

I agree the method would be better; will change.

clarfonthey · 2026-07-14T15:03:29Z

+        // at those fields unconditionally.
+        let range_is_empty = range.is_empty();
+        if *is_inclusive {
+            debug_assert!(range.start <= range.end);


Not to prematurely optimise, but do you think there should be a cold_path() here too? Since is_inclusive should ideally not be present.

View changes since the review

maybe

#[inline] fn is_inclusive(&self) -> bool { unlikely(self.is_inclusive) }

That would have to be inline(always) to work properly and I'm still a bit sceptical that's the right approach.

Maybe not the method, but this does feel like one of the cases where unlikely is more natural than cold_path. But maybe we prefer the stable option.

It feels natural, but it is unlikely to achieve the desired effect, which is the main issue. These sort of micro-optimisations are very fickle, and part of the reason for using cold_path is reliability with achieving the desired effect.

The thing that makes me uncertain for this is that the only hammer we have are branch edge weights.

So if LLVM dutifully listened to us and outlined (via a predictable-to-be-false forward branch) the return false from this arm, would that actually be better? I'm not convinced. I think I'd rather the compiler do the Range::is_empty and just bit-and it with is_inclusive, since it's forced to read that bool anyway.

(I also tried writing this function as range.is_empty() & !is_inclusive, but that failed the make_ord_iter_check_empty test. Thus the branch-but-easily-unbranchable phrasing here -- plus the branch gives a nice spot to put a debug_assert.)

To be clear, I think it would be fair to just leave a comment how cold_path didn't really help here as a future note so if we ever revisit it we know what the previous analysis is. Ditto for the other discussion, my thought process is basically: does this help? If not, fair, but we should document that.

clarfonthey · 2026-07-14T15:06:15Z

-        Some(RangeInclusive { start: self.0.start, last: self.0.end })
+        let Self { range: legacy::Range { start, end }, is_inclusive } = self;
+        let last = if is_inclusive {
+            end


Ditto here for cold_path().

View changes since the review

Similar questions came to mind to me here as in the other spot, so I wasn't sure what was best.

For example, in (x..=255_u8).into_iter().remainder(), do I really even want a branch? Even with a prediction hint, I don't know that branching on the bool is better than having it just end - (!is_inclusive as u8). (Similar questions applied in size_hint, though there there's an argument that the branch hint would be worth it because saturating_add+checked_add isn't totally trivial, so it might be more worth it anyway.)

But also, in next the branch for is_inclusive is definitely cold, even if every single one of your RangeInclusiveIters happens to be inclusive (well, assuming they're not just all MAX..=MAX). But it would be possible for a branch here in remainder (and similarly in is_empty) to actually not be cold at all because the ranges really did hit the end of the range often.

clarfonthey · 2026-07-14T15:07:45Z

Why { self } instead of just self?

View changes since the review

Because calling next needs &mut.

(I have a habit of preferring this over changing the signature to mut self, but could go either way.)

Oh, I had no idea this cursed trick worked. I just haven't seen it before is all. You can keep it if you prefer.

clarfonthey · 2026-07-14T15:10:32Z

+                // This is unreachable for `Ord` types, but `Step` accepts partial orders.
+                // So it's possible for the range to be empty even if `last` is
+                // a maximal element in the DAG.
+                debug_assert_eq!(PartialOrd::partial_cmp(&start, &last), None);


Don't care too much, but perhaps rather than resorting to the debug_assert_eq! within an if, maybe combine it into the assert condition?

View changes since the review

clarfonthey · 2026-07-14T15:11:03Z

    }
 }

+// tidy-alphabetical-start


Just wanna show appreciation whenever people do this. <3

View changes since the review

clarfonthey · 2026-07-14T15:14:24Z

+    }
+}
+
+/// A type that's a valid `Step` but isn't `Ord`


You can read the code to find out, but I would further clarify here that the variants only compare with themselves, and that's how the PartialOrd implementation is done.

This is also a minor preference that you don't need to go with, but I think it would make more sense to put this definition above the code that uses it for better readability, since you need to know how this works to understand the tests.

View changes since the review

clarfonthey · 2026-07-14T15:15:42Z

r=me minus a few, mostly non-blocking concerns.

rustbot · 2026-07-22T05:20:12Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot assigned clarfonthey Jul 13, 2026

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 13, 2026

scottmcm force-pushed the improved-rangeinclusive-iter branch from 78f5932 to a058e3b Compare July 13, 2026 03:49

This comment has been minimized.

Sign in to view

scottmcm force-pushed the improved-rangeinclusive-iter branch from a058e3b to 7e9ef25 Compare July 13, 2026 04:39

clarfonthey reviewed Jul 14, 2026

View reviewed changes

scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2026

[new RangeInclusive] Convert to half-open in into_iter if possible

e05602b

scottmcm force-pushed the improved-rangeinclusive-iter branch from 7e9ef25 to e05602b Compare July 22, 2026 05:20

	fmt::Formatter::debug_tuple_field1_finish(f, "RangeInclusiveIter", field)
	f.debug_tuple_field1_finish("RangeInclusiveIter", field)

Uh oh!

Uh oh!

Conversation

scottmcm commented Jul 13, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

clarfonthey Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey commented Jul 14, 2026

Uh oh!

rustbot commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

scottmcm commented Jul 13, 2026 •

edited by rustbot

Loading

clarfonthey Jul 14, 2026 •

edited by rustbot

Loading

clarfonthey Jul 14, 2026 •

edited by rustbot

Loading

clarfonthey Jul 14, 2026 •

edited by rustbot

Loading

clarfonthey Jul 14, 2026 •

edited by rustbot

Loading

clarfonthey Jul 14, 2026 •

edited by rustbot

Loading

clarfonthey Jul 14, 2026 •

edited by rustbot

Loading

clarfonthey Jul 14, 2026 •

edited by rustbot

Loading

clarfonthey Jul 14, 2026 •

edited by rustbot

Loading