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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.

Generally looks good, thanks for doing this cleanup. I agree we should unconditionally define the various typedefs everywhere to remove some `#if`s.



================
Comment at: libcxx/include/__memory/raw_storage_iterator.h:41-47
+#if _LIBCPP_STD_VER > 14
+  using iterator_category = output_iterator_tag;
+  using value_type = void;
+  using difference_type = void;
+  using pointer = void;
+  using reference = void;
+#endif
----------------
Quuxplusone wrote:
> Here, we //could// drop the `#if` and just unconditionally define these members... except that that would visibly change the value of `raw_storage_iterator::difference_type` (from non-conforming to conforming), and maybe we don't want that? But again I'd think we don't really care about this type and so we might as well lose the `#if`.
I think it's an acceptable change - I doubt it will break many users since `raw_storage_iterator` isn't widely used, and it's a compile-time breakage in all cases, so not some nasty runtime issue.


================
Comment at: libcxx/include/iterator:85
+struct iterator                                            // until C++17
+struct [[deprecated)]] iterator                            // since C++17
 {
----------------
cjdb wrote:
> 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.
Please use `// deprecated in C++17` instead -- that is what we do elsewhere.


================
Comment at: libcxx/test/libcxx/iterators/iterator.requirements/iterator.concepts/cpp20_iter_concepts.pass.cpp:11-12
 
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+
----------------
Why do we need this? Aren't we removing the use of those deprecated decls?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101729