This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Deprecate std::iterator and remove it as a base class
ClosedPublic

Authored by ldionne on May 26 2021, 8:29 AM.

Details

Summary

C++17 deprecated std::iterator and removed it as a base class for all
iterator adaptors. We implement that change, but we still provide a way
to inherit from std::iterator in the few cases where doing otherwise
would be an ABI break.

Supersedes D101729 and the std::iterator base parts of D103101 and D102657.

Diff Detail

Event Timeline

ldionne requested review of this revision.May 26 2021, 8:29 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 8:29 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

SGTM some questions.

libcxx/include/__memory/raw_storage_iterator.h
45

Is it intended to change the types of these typedefs and the base class?

libcxx/include/iterator
510

Can we remove this class when _LIBCPP_ABI_NO_ITERATOR_BASES is defined?

LGTM % comments!

libcxx/include/iterator
510

No. The existence of this class doesn't have ABI implications. Also, this class is still present (but deprecated) in C++20. https://timsong-cpp.github.io/cppwp/n4861/depr.iterator.basic#lib:iterator
If this class were removed from C++23, we'd want to guard it under an API macro such as _LIBCPP_ENABLE_CXX23_REMOVED_ITERATOR.

818

D103101 updates this code for C++20 (where difference_type becomes ptrdiff_t instead of void). Do you want to roll that in here á là D103101, or would you like a followup PR for that?
Notice that we want to change the value of the typedef in C++20, but not alter the name of the base class (because that would break ABI).

libcxx/test/libcxx/iterators/iterator.requirements/iterator.concepts/cpp20_iter_concepts.pass.cpp
48–62

These changes look OK to me, but it would be useful to know what the author was originally trying to test here: this test didn't have anything to do with std::iterator's behavior, did it?
I suggest drive-by changing

namespace std {
template <>
struct iterator_traits<MyIter3> {

to

template <>
struct std::iterator_traits<MyIter3> {

both to save LOC and to avoid reopening namespace std.

libcxx/test/libcxx/iterators/iterator.requirements/iterator.concepts/cpp20_iter_traits.pass.cpp
0

Same comment here about reopening namespace std.
But also here it is clear that MyIter and MyIter2 are exactly equivalent, and so you can eliminate one of them.
It looks like the point of this test is to verify that std::_ITER_TRAITS<T> is std::iterator_traits<T> when T specializes iterator_traits, and T when it doesn't. So the entire test file should look like

#include <iterator>
#include "test_iterators.h"
struct A : random_access_iterator<int*> {};
struct B : random_access_iterator<int*> {};
struct C : random_access_iterator<int*> {};
struct D : random_access_iterator<int*> {};
template<> struct std::iterator_traits<B> {};
template<> struct std::iterator_traits<C> : std::iterator_traits<A> {};
template<> struct std::iterator_traits<D> : std::iterator_traits<int*> {};

static_assert(std::is_same_v<std::_ITER_TRAITS<int*>, std::iterator_traits<int*>>);
static_assert(std::is_same_v<std::_ITER_TRAITS<A>, A>);
static_assert(std::is_same_v<std::_ITER_TRAITS<B>, std::iterator_traits<B>>);
static_assert(std::is_same_v<std::_ITER_TRAITS<C>, std::iterator_traits<C>>);
static_assert(std::is_same_v<std::_ITER_TRAITS<D>, std::iterator_traits<D>>);

and it should be a .compile.pass.cpp test.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iterator/types.pass.cpp
54

Probably should say typename R::value_type instead of char, for consistency with lines 49-53.

ldionne updated this revision to Diff 348071.May 26 2021, 1:28 PM
ldionne marked 7 inline comments as done.

Apply review feedback

ldionne added inline comments.
libcxx/include/__memory/raw_storage_iterator.h
45

Yes, I'm making it standards conforming. I'll add a test too, it seems like we didn't have any.

libcxx/include/iterator
510

That's all correct.

However, going forward, I'll be keen to remove things without guarding them with something like _LIBCPP_ENABLE_CXX23_REMOVED_ITERATOR: if you opt-in to a newer standard mode, it means you're OK with the removal. So, for example, I'd be fine if we had not guarded the removal of std::raw_storage_iterator by _LIBCPP_ENABLE_CXX20_REMOVED_RAW_STORAGE_ITERATOR, but it's fine too cause it's done now.

818

I was planning on a different review where I update all the iterator types like you did in D103101 (and @zoecarver in his own similar patch).

libcxx/test/libcxx/iterators/iterator.requirements/iterator.concepts/cpp20_iter_concepts.pass.cpp
48–62

I think it's safe to assume there was no intent to test std::iterator specifically here. Agreed about the drive-by change.

libcxx/test/libcxx/iterators/iterator.requirements/iterator.concepts/cpp20_iter_traits.pass.cpp
0

Yup, I agree this is more straightforward.

Quuxplusone accepted this revision.May 26 2021, 2:51 PM

LGTM, ship it!
Will still need a followup to fix the difference_type of the output iterators for C++20. Are you doing that, or am I repurposing D103101, or what?

libcxx/include/__config
97–99

I wish this list were alphabetized, but it's already not-alphabetized, so meh.

libcxx/include/__memory/raw_storage_iterator.h
45

This is also, technically, an ABI break ;) but you're right, it fixes a bug that's apparently been undetected forever.

libcxx/test/std/iterators/stream.iterators/istream.iterator/types.pass.cpp
33

I think you don't need this because std::iterator wasn't deprecated <=14 and now you're mentioning it only under #if TEST_STD_VER <= 14.
Presumably ditto in some other files as well.

This revision is now accepted and ready to land.May 26 2021, 2:51 PM
libcxx/include/__memory/raw_storage_iterator.h
34–36

Actually, one last thing I don't understand: What's the purpose of _LIBCPP_SUPPRESS_DEPRECATED_PUSH here, given that this entire file is already controlled by #pragma GCC system_header? Do we hit situations where we're actually seeing deprecation diagnostics from this and other system headers?
I know you didn't invent _LIBCPP_SUPPRESS_DEPRECATED_PUSH, so it must be solving some problem, but I don't get it.

ldionne updated this revision to Diff 348236.May 27 2021, 6:08 AM
ldionne marked 4 inline comments as done.

Apply review comments and fix GCC build.

ldionne added inline comments.May 27 2021, 8:31 AM
libcxx/include/__memory/raw_storage_iterator.h
34–36

We disable #pragma GCC system_header when we run the test suite to avoid missing some potentially important warnings/errors, so we need to disable this warning locally to avoid failing the test suite.

45

I agree, but I don't care about this one. I think literally nobody is ever going to be affected.

libcxx/test/std/iterators/stream.iterators/istream.iterator/types.pass.cpp
33

Good catch, thanks.