This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix signed overflow inside ranges::advance.
ClosedPublic

Authored by Quuxplusone on Jul 23 2021, 7:20 PM.

Details

Reviewers
ldionne
cjdb
Group Reviewers
Restricted Project
Commits
rG41b17c444df6: [libc++] Fix signed overflow inside ranges::advance.
Summary
See LWG reflector thread titled 'Question on ranges::advance and "past-the-sentinel iterators"',
from 2021-07-23.
Test case heavily based on one graciously provided by Casey Carter.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jul 23 2021, 7:20 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJul 23 2021, 7:20 PM

whoops, didn't commit --amend before diffing

cjdb requested changes to this revision.Jul 23 2021, 7:49 PM
cjdb added inline comments.
libcxx/include/__iterator/advance.h
79

Please don't remove noexcept.

156

These explicit conversions shouldn't be necessary: zero is zero for all integer-like types. I also don't think swapping these two lines around is going to benefit anything.

161

Please don't change the type.

164

This else is redundant, please remove it.

172

These explicit conversion-to-bools aren't necessary.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
104

Please match the naming conventions that the file uses.

162

I'd prefer we use std::numeric_limits<Iota::difference_type>::min().

This revision now requires changes to proceed.Jul 23 2021, 7:49 PM

Address review comments.

ldionne accepted this revision.Jul 26 2021, 6:11 AM

Thanks for fixing this. LGTM but please check whether @cjdb is happy too.

cjdb added a comment.Jul 26 2021, 9:39 AM

Thanks for fixing this. LGTM but please check whether @cjdb is happy too.

It's almost ready, but I think my comments for the test file were missed.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
104

Got any suggestions? The "naming convention" in this file seems no more than "something long with lots of underscores."

cjdb added inline comments.Jul 26 2021, 11:27 AM
libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
104

I'm fine with just iota, but if you want something "more", perhaps iota_range, or iota_range_regression.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 26 2021, 1:42 PM
This revision was automatically updated to reflect the committed changes.