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

[Louis Dionne via Phabricator via llvm-commits]

ldionne marked 4 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/__config:988
 #if _LIBCPP_STD_VER > 11
-#  define _LIBCPP_DEPRECATED [[deprecated]]
+#  define _LIBCPP_DEPRECATED __attribute__((deprecated))
 #else
----------------
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.


================
Comment at: libcxx/include/__config:993
 
+#if _LIBCPP_STD_VER >= 11
+#  define _LIBCPP_DEPRECATED_IN_CXX11 __attribute__((deprecated))
----------------
EricWF wrote:
> So here's a quirk you'll need to learn.
> 
> `_LIBCPP_STD_VER` is never less than 11, even in C++03 mode, because libc++ attempts to provide a feature complete C++11 implementation regardless of the actual language dialect.
> 
> Therefore, this macro marks C++03 components as deprecated in C++03 :-(
> 
> The macro you want to use to detect pre-C++11 is `defined(_LIBCPP_CXX03_LANG)`.
Thanks. That's weird. I would have assumed that it just matched the `-std=c++xx` specified on the command line, i.e. the language enforced by the compiler.


================
Comment at: libcxx/include/__functional_base:27
 
+// TODO(ldionne): Mark this as deprecated in C++11 and remove in C++17.
 template <class _Arg1, class _Arg2, class _Result>
----------------
EricWF wrote:
> No `TODO`s in headers please, unless the intent is to remove them before the end of the review.
> Instead, you should file a bug regarding the work to be done.
> 
> 
> I know I've been guilty of this, but it's never too late to change.
I saw your TODOs and so I decided to add mine ;-). Filed https://bugs.llvm.org/show_bug.cgi?id=38054 instead.


================
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"
----------------
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.



Repository:
  rL LLVM

https://reviews.llvm.org/D48912