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

[Takuya Shimizu via Phabricator via libcxx-commits]

hazohelet added a comment.

Thanks for the feedback.



================
Comment at: clang/test/SemaCXX/warn-constant-evaluated-constexpr.cpp:38
 constexpr int fn5() {
-  if constexpr (__builtin_is_constant_evaluated()) // expected-warning {{'__builtin_is_constant_evaluated' will always evaluate to 'true' in a manifestly constant-evaluated expression}}
+  if constexpr (__builtin_is_constant_evaluated()) // expected-warning {{'__builtin_is_constant_evaluated' will always evaluate to true in this context}}
     return 0;
----------------
cjdb wrote:
> This should also generate a fix-it hint, since we can automate the fix.
I think it's reasonable to show fix-it hint to remove `constexpr` here.
For that we need to store the source range of `constexpr` in evaluation context record if it's constexpr-if condition.


================
Comment at: clang/test/SemaCXX/warn-tautological-meta-constant.cpp:18
+  } else {
+    if constexpr (std::is_constant_evaluated()) { // expected-warning {{always evaluate to true}}
+      return 0;
----------------
cjdb wrote:
> I'm not a fan of this diagnostic text: it doesn't offer insight into what's gone wrong or provide actionable feedback on how to fix the code.
I agree that the current wording (`... will always evaluate to true in this context`) is not great, but I'm not sure how to improve it because the reason to see this diagnostic would mostly be a misunderstanding of the C++ const semantics.
At least, if the appearance of `is_constant_evaluated` is in the condition of constexpr-if, probably we can say they should remove `constexpr`.
CC @aaron.ballman 


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

https://reviews.llvm.org/D155064