[libcxx-commits] [PATCH] D101729: [libcxx] deprecates `std::iterator` and removes it as a base class

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb planned changes to this revision.
cjdb added a comment.

Haven't had time to respond to changes (and this is lowish in my p-queue).



================
Comment at: libcxx/include/iterator:85
+struct iterator                                            // until C++17
+struct [[deprecated)]] iterator                            // since C++17
 {
----------------
Quuxplusone wrote:
> Stray `)` here. But please remove this diff anyway; `[[deprecated]]` doesn't appear in the Standard and even if it did I don't think it needs to appear in the synopsis comments.
Let's take this one to Discord, I have some thoughts on this.


================
Comment at: libcxx/include/iterator:629-631
+#if _LIBCPP_STD_VER > 14
+    typedef typename iterator_traits<_Iter>::value_type      value_type;
+#endif
----------------
Quuxplusone wrote:
> Lose the `#if` here.
> (Any idea why the old code bothered to redundantly redefine `difference_type` etc.?)
According to https://timsong-cpp.github.io/cppwp/n4140/reverse.iterator, it's standard to do this :S
I'm more curious to know why `value_type` is missing than why the others were redundantly added.

Does this change your opinion at all?


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iterator/types.pass.cpp:56
+
+    using iterator_base = std::iterator<typename T::iterator_category, char>;
+#if TEST_STD_VER <= 14
----------------
Quuxplusone wrote:
> This isn't right when `T::iterator_category` is `contiguous_iterator_tag`, although maybe the test doesn't care.
> This also isn't right when `T::difference_type` isn't `ptrdiff_t`, etc.; but I'm sure the test doesn't care.
> This isn't right when `T::iterator_category` is `contiguous_iterator_tag`, although maybe the test doesn't care.
> This also isn't right when `T::difference_type` isn't `ptrdiff_t`, etc.; but I'm sure the test doesn't care.

I'm inclined to say the test doesn't care today, but it ~~will~~  should care when we get around to applying P0896. I've got no qualms punting this to future me, but since I wager an even chance that @zoecarver will beat me to it, I'd like his input before making that decision.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101729/new/

https://reviews.llvm.org/D101729