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

[A. Jiang via Phabricator via libcxx-commits]

frederick-vs-ja added a comment.

In D145376#4179548 <https://reviews.llvm.org/D145376#4179548>, @ldionne wrote:

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

I do believe diagnostic is required (otherwise, implementations are allowed to ignore "//Mandates//:").

1. [declval]/2 uses "//Mandates//:";
2. [structure.specifications]/3.2 says that violation of "//Mandates//:" make the program ill-formed;
3. [intro.compliance.general] implies that unless "no diagnostic required" is explicitly mentioned, diagnostic is required for ill-formedness.


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

https://reviews.llvm.org/D145376