[libcxx-commits] [PATCH] D103171: [libc++] Deprecate std::iterator and remove it as a base class

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__memory/raw_storage_iterator.h:45
+    typedef void                pointer;
+    typedef void                reference;
+
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Mordante wrote:
> > > Is it intended to change the types of these typedefs and the base class?
> > Yes, I'm making it standards conforming. I'll add a test too, it seems like we didn't have any.
> This is also, technically, an ABI break ;) but you're right, it fixes a bug that's apparently been undetected forever.
I agree, but I don't care about this one. I think literally nobody is ever going to be affected.


================
Comment at: libcxx/include/__memory/raw_storage_iterator.h:33-35
+_LIBCPP_SUPPRESS_DEPRECATED_PUSH
+    : public iterator<output_iterator_tag, void, void, void, void>
+_LIBCPP_SUPPRESS_DEPRECATED_POP
----------------
Quuxplusone wrote:
> 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.
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.


================
Comment at: libcxx/test/std/iterators/stream.iterators/istream.iterator/types.pass.cpp:35
 
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+
----------------
Quuxplusone wrote:
> 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.
Good catch, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103171