[PATCH] D48912: [libc++] Add deprecated attributes to many deprecated components

[Eric Fiselier via Phabricator via llvm-commits]

EricWF added inline comments.


================
Comment at: libcxx/include/__config:988
+// Deprecations warnings are only enabled when _LIBCPP_ENABLE_DEPRECATION_WARNINGS is defined.
+#if defined(_LIBCPP_ENABLE_DEPRECATION_WARNINGS) && __has_attribute(deprecated)
 #  define _LIBCPP_DEPRECATED [[deprecated]]
----------------
We should be able to assume that we have the C++11 form of `[[deprecated]]` without checking `__has_attribute(deprecated)`, which only works for Clang, and is really checking for `__attribute__((deprecated))` IIRC.

Also, another possibility, which is in opposition to @mclow.lists, and this would be my suggestion, is to enable it by default, and then allow users to opt out simply by defining `-D_LIBCPP_DEPRECATED`.

```
#ifndef _LIBCPP_DEPRECATED
# define _LIBCPP_DEPRECATED [[deprecated]]
#endif
```

But we have to agree to enable them by default first.


================
Comment at: libcxx/include/__config:988
 #if _LIBCPP_STD_VER > 11
-#  define _LIBCPP_DEPRECATED [[deprecated]]
+#  define _LIBCPP_DEPRECATED __attribute__((deprecated))
 #else
----------------
ldionne wrote:
> EricWF wrote:
> > Why are we switching the attribute from its C++11 form?
> > 
> > I suspect this will break MSVC builds, or other compilers that don't provide GNU attributes.
> This macro was not being used anywhere, and for some reason I got compilation errors when using the C++11 form. I've now tracked it down to the `[[deprecated]]` attribute appearing _before_ the visibility attribute. Indeed, the following won't work ([live example](https://wandbox.org/permlink/ejmkTr7rNKNb2Ma1)):
> 
> ```
> template<class _Tp>
> class [[deprecated]] __attribute__((__type_visibility__("default"))) auto_ptr { };
> ```
> 
> But the following does ([live example](https://wandbox.org/permlink/3XEQSxH0KH9jLf7A)):
> 
> ```
> template<class _Tp>
> class __attribute__((__type_visibility__("default"))) [[deprecated]] auto_ptr { };
> ```
> 
> I'll go back to the C++11 form and put the deprecated attributes in the right spot.
> 
> Also, do we have testers on MSVC? Where can I see those? More generally, do we have something like a Travis/Appveyor CI setup I could take a look at? This would be invaluable.
Yeah, C++11 attributes are a lot less tolerant about where they can appear, and so they can be a pain to apply initially.

The builders we currently have are listed here: http://libcxx.llvm.org/docs/#build-bots-and-test-coverage
The Appveyor bots are not currently passing, but the idea is to at least keep them from regressing.



================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.cxx1z.pass.cpp:26
 
+#ifdef __clang__
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
----------------
ldionne wrote:
> EricWF wrote:
> > How do these tests work with compilers other than Clang?
> > 
> > Perhaps you can add a `disable_deprecated_declarations.h` header, and include that instead? That way the one header can be made to handle most compilers.
> They must always break because we compile with `-Wall -Werror`. I thought the tests were only supposed to work with Clang -- I copied this pattern from another test (`test/libcxx/containers/gnu_cxx/hash_map.pass.cpp`).
> 
> Per Marshall's comment, those diagnostics will be disabled by default, so I won't need this header (or this pragma) anymore. I'll see if it makes sense to add the header for the other tests that depend on this pragma, and if so, I'll add it in another Phab review.
> 
The tests are actually expected to work with GCC, Clang, and MSVC (to varying degrees).

The MSVC team actually uses our test suite to test their STL, and so all tests under `test/std` should be written with that in mind.
However, tests under `test/libcxx`, as these are, only need to work against libc++ (but with both GCC and Clang except where it's not possible).

After investigating `hash_map.pass.cpp` it seems to be by accident that the test doesn't fail under GCC. GCC doesn't promote `#warning` to an error even when `-Werror` is specified. Additionally `-W#warnings` is a Clang specific flag. GCC's version is `-Wcpp`.

However, these tests use `[[deprecated]]` not `#warning` to produce the diagnostic, and so must be made to work with GCC.


Repository:
  rL LLVM

https://reviews.llvm.org/D48912