[libcxx-commits] [PATCH] D68879: P1152R4: Fix deprecation warnings in libc++ testsuite and in uses of is_invocable that would internally conjure up a deprecated function type.

[Richard Smith - zygoloid via Phabricator via libcxx-commits]

rsmith marked an inline comment as done.
rsmith added inline comments.


================
Comment at: libcxx/include/type_traits:1123
+// Suppress deprecation notice for volatile-qualified return type.
+_LIBCPP_SUPPRESS_DEPRECATED_PUSH
 template <class _Tp> _Tp&& __declval(int);
----------------
ldionne wrote:
> rsmith wrote:
> > ldionne wrote:
> > > rsmith wrote:
> > > > ldionne wrote:
> > > > > Wait, I don't see any `volatile` qualification here?
> > > > 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.
> > > 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.
> > 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?
> My question was: why don't we warn in the location where the user says `std::declval<volatile Foo>()`, and only there?
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` `_Tp`s 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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68879