[libcxx-commits] [PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__type_traits/is_constant_evaluated.h:31
+  return false;
+#endif
 }
----------------
Why is this needed? Does this mean the builtin will fail in C++03 mode?


================
Comment at: libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp:27
   static_assert(!std::is_constant_evaluated(), "");
-  // expected-warning at -1 0-1 {{'std::is_constant_evaluated' will always evaluate to 'true' in a manifestly constant-evaluated expression}}
+  // expected-warning at -1 0-1 {{'std::is_constant_evaluated' will always evaluate to true in this context}}
 #endif
----------------
philnik wrote:
> cor3ntin wrote:
> > Mordante wrote:
> > > hazohelet wrote:
> > > > philnik wrote:
> > > > > Mordante wrote:
> > > > > > Since libc++ support the latest ToT Clang and the last two official releases this wont work. The `expected-warning` needs to be a `expected-warning-re` that works for both the new and old diagnostic
> > > > > You can also just shorten it to `'std::is_constant_evaluated' will always evaluate to`. Seems good enough to me.
> > > > Thanks!
> > > I really would like a regex. To me the current message misses an important piece of information; the `true` part. I care less about the rest of the message, but stripping the `true` means a warning like `std::is_constant_evaluated' will always evaluate to FALSE` would be valid too.
> > Agreed with Mordante
> We're not in the business of testing the compiler though. Taking a closer look, I'm not actually sure why this test exists at all. It doesn't seem like it tests anything useful w.r.t. the library. This has been added in 2fc5a78, but there the warning isn't checked, so that was clearly not the original intention.
> We're not in the business of testing the compiler though. Taking a closer look, I'm not actually sure why this test exists at all. It doesn't seem like it tests anything useful w.r.t. the library. This has been added in 2fc5a78, but there the warning isn't checked, so that was clearly not the original intention.

I agree. But to me this test tests whether 


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

https://reviews.llvm.org/D155064