[libcxx-commits] [PATCH] D145376: [libc++] add declval failure assertion for instantiation

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D145376#4179024 <https://reviews.llvm.org/D145376#4179024>, @EricWF wrote:

> Why?
>
> We went out of our way to make `declval` as low cost as we possibly could. Adding a body for the compiler to see is just work.

Well, while this is technically a QOI issue (since ODR-using `declval` makes the program ill-formed no-diagnostic-required), we usually strive to provide nice error messages for users, when we can. So this seems desirable to me (assuming that doesn't make our implementation incorrect, obviously).

Summarizing the situation, it looks like Clang (with libstdc++) and GCC (with libstdc++) both reject valid code right now: https://gcc.godbolt.org/z/4Kb9qG5e5
However, what we're trying to achieve is that Clang (with libc++) does not accept this invalid code: https://gcc.godbolt.org/z/aPKjb7odo

I think we should just file a bug report against Clang and GCC and get those fixed, and then we can proceed with this change. It seems weird to me that the body of the function is instantiated when trying to check for narrowing, since we are providing the return type of `declval` explicitly. That just seems like a bug that could bite in other scenarios to me. And once Clang and GCC don't try to instantiate the body of `declval`, I don't think there would be performance concerns anymore (@EricWF do you disagree?).



================
Comment at: libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp:21
   std::ostringstream s;
+#ifndef TEST_HAS_NO_CHAR8_T
+  char8_t c8_s[]       = u8"test";
----------------
This is a nice catch! If this patch gets stuck, I would consider committing this separately since it's just a NFC fix to a test that is clearly incorrect.


================
Comment at: libcxx/test/std/utilities/utility/declval/declval.verify.cpp:15-23
+int main() {
+  if (false) {
+    int i = std::declval<int>(); // expected-warning@*:* {{code will never be executed}}
+    (void)i;
+  }
+
+  return 0;
----------------
I think you could just do this:

```
int x = std::declval<int>(); // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed{{.*}}Calling declval is ill-formed, see [declval]/2.}}
```

There is no need to have a `main` function in a `.verify.cpp` test, since it only compiles the code but doesn't try to link or run it!


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

https://reviews.llvm.org/D145376