This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][ranges] Fix tests for stdlib types that conform to sized_sentinel_for.

Authored by zoecarver on Apr 27 2021, 9:04 AM.

Diff Detail

Event Timeline

zoecarver requested review of this revision.Apr 27 2021, 9:04 AM
zoecarver created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 9:04 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.

You mean !std::sized_sentinel_for, since these iterators are merely bidirectional. (Sized-sentinel-ness essentially requires random-access iterators, because it requires subtraction to work.)

zoecarver updated this revision to Diff 340973.Apr 27 2021, 1:39 PM
  • Fix ordering of tests too.
cjdb accepted this revision.Apr 27 2021, 2:26 PM

LGTM pending CI passing. This was a bit smaller than I'd expected, so please do a quick check to make sure you caught them all before submitting. Thanks for following up on this!

zoecarver updated this revision to Diff 340997.Apr 27 2021, 3:11 PM
  • Expect map, multimap not to be sized_sentinel_for.
zoecarver updated this revision to Diff 341023.Apr 27 2021, 4:35 PM
  • Bump the buildbots.
Quuxplusone accepted this revision.Apr 28 2021, 7:21 AM

Ship it, since it fixes typos in the tests.

But, like, none of these tests are useful, IMO. It's arguably useful to test in one place (namely in the reverse_iterator tests) that declval<T>() == declval<reverse_iterator<T>>() is SFINAE-friendly ill-formed; but testing it indirectly, via the same 4 copied lines, pasted among 10 different container tests, and only tested in C++20 mode, is not so useful.

This revision is now accepted and ready to land.Apr 28 2021, 7:21 AM