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

Implement a specialized version std::iter::Enumerate for TrustedLen #77822

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add explanation why Add::add() is called directly instead of + in the…
… Enumerate implementation
  • Loading branch information
sdroege committed Jan 30, 2021
commit 44c5554dacf9597cd2006fe1e57b3b38adc125ef
10 changes: 8 additions & 2 deletions library/core/src/iter/adapters/enumerate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ where
default fn next(&mut self) -> Option<Self::Item> {
let a = self.iter.next()?;
let i = self.count;
// Possible undefined overflow.
// Possible undefined overflow. By directly calling the trait method instead of using the
// `+=` operator the decision about overflow checking is delayed to the crate that does code
// generation, even if overflow checks are disabled for the current crate. This is
// especially useful because overflow checks are usually disabled for the standard library.
AddAssign::add_assign(&mut self.count, 1);
Some((i, a))
}
Expand All @@ -82,6 +85,7 @@ where
// SAFETY: the caller must uphold the contract for
// `Iterator::__iterator_get_unchecked`.
let value = unsafe { try_get_unchecked(&mut self.iter, idx) };
// See comment in `next()` for the reason why `Add::add()` is used here instead of `+`.
(Add::add(self.count, idx), value)
}

Expand Down Expand Up @@ -123,7 +127,8 @@ where
intrinsics::assume(self.count < self.len);
}
let i = self.count;
// Possible undefined overflow.
// See comment in `next()` of the default implementation for the reason why
// `AddAssign::add_assign()` is used here instead of `+=`.
AddAssign::add_assign(&mut self.count, 1);
Some((i, a))
}
Expand All @@ -136,6 +141,7 @@ where
// SAFETY: the caller must uphold the contract for
// `Iterator::__iterator_get_unchecked`.
let value = unsafe { try_get_unchecked(&mut self.iter, idx) };
// See comment in `next()` for the reason why `Add::add()` is used here instead of `+`.
let idx = Add::add(self.count, idx);
Copy link
Member

Choose a reason for hiding this comment

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

Why does this use Add::add instead of +? If this is necessary, a comment explaining why would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how it was originally and is everywhere else in this file. I don't know why and was wondering the same, but kept it because there presumably was a reason

Copy link
Contributor

Choose a reason for hiding this comment

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

The Add::add delays decision about overflow check mode to the crate that does the code generation, even if overflow checks are disabled in the current crate, which they usually are for the standard library.

Copy link
Member

Choose a reason for hiding this comment

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

According to #81721, we should be using #[rustc_inherit_overflow_checks] with a regular + operator here instead of the Add:add trick.

// SAFETY: There must be fewer than `self.len` items because of `TrustedLen`'s API contract
unsafe {
Expand Down