Page MenuHomePhabricator

P1152R4: Fix deprecation warnings in libc++ testsuite and in uses of is_invocable that would internally conjure up a deprecated function type.
ClosedPublic

Authored by rsmith on Oct 11 2019, 11:00 AM.

Diff Detail

Event Timeline

rsmith created this revision.Oct 11 2019, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 11:00 AM
ldionne added inline comments.Oct 11 2019, 12:19 PM
libcxx/include/__config
966

Please document that we're talking about compiler deprecation warnings, otherwise this can be confused as also turning off library deprecation warnings. Ignoring -Wdeprecated won't have that effect, right?

// Macros to enter and leave a state where compiler deprecation warnings are suppressed. This doesn't impact library deprecation warnings.

Or something along those lines.

969

Please indent nested #defines:

#  define _LIBCPP_SUPPRESS_DEPRECATED_PUSH
libcxx/include/type_traits
1123

Wait, I don't see any volatile qualification here?

rsmith updated this revision to Diff 224671.Oct 11 2019, 1:31 PM
rsmith marked 3 inline comments as done.

Indent nested preprocessor directives.

rsmith updated this revision to Diff 224672.Oct 11 2019, 1:33 PM

Extend comment to explain how __declval gets a volatile-qualified return type.

libcxx/include/__config
966

Ignoring -Wdeprecated turns off all deprecation warnings, regardless of whether they come from attributes or something else. (This might also be useful if libc++ wants to implement something in terms of deprecated library functionality.)

libcxx/include/type_traits
1123

The volatile comes from _Tp, specified indirectly through various callers that internally use declval. In particular, the is_invocable<volatile T> tests end up here.

Harbormaster completed remote builds in B39454: Diff 224672.
ldionne added inline comments.Oct 11 2019, 2:46 PM
libcxx/include/type_traits
1123

That's what I thought, but isn't that warning broken then? Isn't any template going to produce a warning when instantiated with a volatile T? I must be misunderstanding something.

rsmith marked an inline comment as done.Oct 11 2019, 5:51 PM
rsmith added inline comments.
libcxx/include/type_traits
1123

We warn when instantiating the second overload of __declval with _Tp = volatile T, because that causes it to have a volatile-qualified return type, which is deprecated. We don't warn on the first overload, because it doesn't have a volatile-qualified return type regardless of _Tp. Does that make more sense?

davezarzycki added a comment.EditedOct 12 2019, 2:26 AM

Is there an ETA for this fix? Without it, stage two testing fails:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20191007/290336.html

ldionne accepted this revision.Oct 16 2019, 9:15 AM

Let's not block this because I have questions about the warning implemented in Clang. Richard, you can ship this. Thanks for the fix and sorry for the delay.

libcxx/include/type_traits
1123

My question was: why don't we warn in the location where the user says std::declval<volatile Foo>(), and only there?

This revision is now accepted and ready to land.Oct 16 2019, 9:15 AM
Closed by commit rGa9727033fb5f: P1152R4: Fix deprecation warnings in libc++ testsuite and in uses of… (authored by Richard Smith <richard-llvm@metafoo.co.uk>). · Explain WhyOct 18 2019, 5:10 PM
This revision was automatically updated to reflect the committed changes.
rsmith marked an inline comment as done.Oct 18 2019, 5:12 PM
rsmith added inline comments.
libcxx/include/type_traits
1123

The thing that's deprecated is declaring a function with a volatile-qualified function type, so we warn at the point where that happens, which is here, inside the implementation of declval. (This warning will be "in instantiation of" something else, but we use the warning state at the location the warning is pointed at, not the warning state at the point of instantiation, which is why we need the suppression here.)

Note that decltype(_VSTD::__declval<volatile Foo>(0)) (the return type of declval below) will be (non-volatile) Foo if Foo is not a class type, because there are no volatile-qualified non-class (non-array) prvalues. So the volatile gets stripped off before we even get to std::declval. The volatile only appears here temporarily as an implementation detail. Incidentally, the only case when we'll actually select the __declval overload with a volatile-qualified return type is when calling declval<volatile void>(), because all non-volatile _Tps will call the other __declval overload.

This could possibly be fixed in a different way, by rewriting declval to follow the way the standard specifies it:

template<class T>
  add_rvalue_reference_t<T> declval() noexcept;

This would only warn when T is (possibly-const) volatile void, which seems appropriate. However, I suspect the class template instantiation in here would be significantly more expensive than the current implementation. Maybe we could add an __add_rvalue_reference builtin to Clang for use implementing declval?