This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add deprecated attributes to many deprecated components
ClosedPublic

Authored by ldionne on Jul 3 2018, 6:51 PM.

Details

Summary

This is a first step in the right direction, but I wasn't able to get an
exhaustive list of all deprecated components per standard, so there's
certainly stuff that's missing.

rdar://problem/18168350

Diff Detail

Repository
rCXX libc++

Event Timeline

ldionne created this revision.Jul 3 2018, 6:51 PM
ldionne planned changes to this revision.Jul 3 2018, 6:51 PM
EricWF requested changes to this revision.Jul 3 2018, 8:57 PM

We'll also want tests that very the deprecated diagnostics are emitted correctly. To test for the production of diagnostics, Libc++ uses *.fail.cpp tests along with Clang Verify.
I went ahead and implemented [depr.adaptors.cxx11.fail.cpp](https://gist.github.com/EricWF/6355d7d59718fb749c718d3f605cfa9b) which tests that no diagnostics are emitted in C++03, and the correct diagnostics are emitted in C++11 and beyond.

libcxx/include/__config
988 ↗(On Diff #154040)

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.

993 ↗(On Diff #154040)

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).

libcxx/include/__functional_base
27 ↗(On Diff #154040)

No TODOs 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.

libcxx/test/libcxx/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.cxx1z.pass.cpp
26 ↗(On Diff #154040)

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.

Like the "don't discard return values" patch that is currently under review (D45179), this needs to be opt-in - otherwise people will complain (loudly).
I am in favor of doing this - just not by default.

So if someone defines (say) _LIBCXX_DEPRECATION_WARNINGS in their build flags, they get the warnings. Otherwise, not.

ldionne marked 4 inline comments as done.Jul 4 2018, 8:44 AM
ldionne added inline comments.
libcxx/include/__config
988 ↗(On Diff #154040)

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):

template<class _Tp>
class [[deprecated]] __attribute__((__type_visibility__("default"))) auto_ptr { };

But the following does (live example):

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.

993 ↗(On Diff #154040)

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.

libcxx/include/__functional_base
27 ↗(On Diff #154040)

I saw your TODOs and so I decided to add mine ;-). Filed https://bugs.llvm.org/show_bug.cgi?id=38054 instead.

libcxx/test/libcxx/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.cxx1z.pass.cpp
26 ↗(On Diff #154040)

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.

ldionne updated this revision to Diff 154120.Jul 4 2018, 8:50 AM
ldionne marked 4 inline comments as done.

Addressed reviewer feedback.

  • Addressed Eric's inline comments.
  • Added Clang-Verify tests per Eric's comment.
  • Made the warnings opt-in per Marshall's request.

Like the "don't discard return values" patch that is currently under review (D45179), this needs to be opt-in - otherwise people will complain (loudly).
I am in favor of doing this - just not by default.

So if someone defines (say) _LIBCXX_DEPRECATION_WARNINGS in their build flags, they get the warnings. Otherwise, not.

I'm not sure this is a good/correct example.
libcxx does warn by default about nodiscard, it's not opt-in.
What D45179 does, is allows users to opt-in into nodiscard warnings with *earlier* -std=c++xy versions.

FWIW, I would prefer to enable deprecation warnings by default and to have a way of turning them off -- so as to be as close to the standard as possible by default. But I'm fine either way, and this is always a choice we can revisit later by just switching the default from opt-in to opt-out.

EricWF added inline comments.Jul 4 2018, 1:10 PM
libcxx/include/__config
988 ↗(On Diff #154120)

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.

988 ↗(On Diff #154040)

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.

libcxx/test/libcxx/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.cxx1z.pass.cpp
26 ↗(On Diff #154040)

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.

FWIW, I would prefer to enable deprecation warnings by default and to have a way of turning them off

I have a strong preference for this as well.

  • These diagnostics are guaranteed to be true positive by definition, and some of them are even high-value beyond just "by the way, this may be removed" (like std::gets() or std::random_shuffle() for security purposes).
  • Users are not likely to change their build systems to enable diagnostics they believe to already be enabled because they're enabled elsewhere. They may not even /know/ to change their build system because it's hard to notice silent behavior.
  • WG21 has frequently talked about their reliance on tools to tell users about deprecations as early as possible so that they can prepare their code bases for the eventual removal. A default behavior of silencing the deprecation warnings runs contrary to that reliance and makes it harder for users to prepare their code base for those removals.

I think that silencing deprecation warnings by default does users a disservice.

Regarding whether we want to enable or disable those warnings by default: it seems like we've reached consensus (on the LLVM IRC) for enabling the deprecation warnings by default. However, I suggest we keep them disabled by default in this commit, and then change the default in a subsequent change. This will make things easier from an integration perspective, and also in case we need to roll back the defaults because of some unforeseen circumstances.

libcxx/test/libcxx/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.cxx1z.pass.cpp
26 ↗(On Diff #154040)

GCC does not accept [[deprecated]] when mixed with another __attribute__ specifier, which is problematic because most types/functions already have such attributes applied to them for visibility purposes. However, note that applying multiple __attribute__ specifiers seems to work. I'm not sure what to do here -- it seems like the easiest way forward would be to detect when we support __attribute__((deprecated)) on both Clang and GCC, and use that when available. Does that sound like an acceptable way forward?

Regarding whether we want to enable or disable those warnings by default: it seems like we've reached consensus (on the LLVM IRC) for enabling the deprecation warnings by default. However, I suggest we keep them disabled by default in this commit, and then change the default in a subsequent change. This will make things easier from an integration perspective, and also in case we need to roll back the defaults because of some unforeseen circumstances.

This approach sounds fine to me.

ldionne updated this revision to Diff 155478.Jul 13 2018, 1:42 PM

Made sure the attributes worked properly on GCC.

This still requires using attribute((deprecated)) whenever that's
available, both because [[deprecated]] was added only in C++14 (so won't
work for C++11 deprecations), and because of placement issue with GCC.

jfb added a subscriber: jfb.Jul 23 2018, 4:43 PM

In general this lgtm. First comment I think you need to fix, others can go together in a follow-up I think.

libcxx/docs/UsingLibcxx.rst
212 ↗(On Diff #155478)

I thought you'd agreed on IRC to have it defined by default (i.e. enable deprecation warnings by default)?

On-by-default sounds right to me, since that's what the Standard is effectively asking for. I agree we'll make some developers sad, but they'll be even more sad when deprecated features get removed entirely.

libcxx/include/__config
1006 ↗(On Diff #155478)

I assume all of the above will work with deleted things because they'll be behind a macro that #if 0's them when deleted (and then a user can turn the deleted feature back on with another macro?). Or do we just delete things outright?

libcxx/include/memory
2059 ↗(On Diff #155478)

So here's an example where, if you define _LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR you can come back and get auto_ptr and then you get the deprecated warning. Should we have another macro that says (for this one) "deprecated in 11, removed in 17 but you asked to get it back, srlys why?"

ldionne marked an inline comment as done.Jul 26 2018, 11:07 AM

I still think this is good to go as-is, given my answer to JF's comments.

libcxx/docs/UsingLibcxx.rst
212 ↗(On Diff #155478)

The plan was to do this in a follow-up patch. This patch is adding the attributes but not changing behavior, and the followup would only change the default behavior. This is because enabling this by default will require more changes (and thus a larger diff), since some tests will need fixing up, etc.

libcxx/include/__config
1006 ↗(On Diff #155478)

By deleted, you mean removed from the standard? If so, then yes, those attributes are applied to a type/function directly. When a type/function is not declared/defined because it's been removed from the standard, the whole thing is behind a #if 0.

libcxx/include/memory
2059 ↗(On Diff #155478)

Ah, I see what you might have meant above. In the current state of things, you'll get the deprecated warning even if you define _LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR. I think that's the correct thing to do. If you want to get rid of the warnings, just define _LIBCPP_DISABLE_DEPRECATION_WARNINGS (in the future, after the default has been changed). We could do otherwise, but I don't see a strong case for it, and it will complicate things quite a bit because each macro like _LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR will need its own deprecation macro.

jfb accepted this revision.Jul 26 2018, 11:13 AM
ldionne marked an inline comment as done.Sep 22 2018, 11:49 AM

Are we waiting for anything to ship this?

I don't think so. Go nuts!

This revision was not accepted when it landed; it landed in state Needs Review.Sep 24 2018, 7:22 AM
This revision was automatically updated to reflect the committed changes.