[PATCH] D69518: [Diagnostics] Warn for std::is_constant_evaluated in constexpr mode

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

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:333
+def warn_is_constant_evaluated_always_true_constexpr : Warning<
+  "'%0' will always evaluate to 'true' in constexpr context">,
+  InGroup<DiagGroup<"constant-evaluated">>;
----------------
"constexpr context" doesn't mean anything, and I'm not sure it has the right implications (eg, is the body of a constexpr function a constexpr context?).

The technically-correct thing would be "in a manifestly constant-evaluated expression".




================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:334
+  "'%0' will always evaluate to 'true' in constexpr context">,
+  InGroup<DiagGroup<"constant-evaluated">>;
+
----------------
Looks like you decided not to make this a subgroup of tautological-compare? (I'm OK with that, I just wanted to make sure this was intentional given the prior discussion.)


================
Comment at: clang/lib/AST/ExprConstant.cpp:10595
+    const auto *Callee = Info.CurrentCall->getCallee();
+    if (Info.InConstantContext && !Info.CheckingPotentialConstantExpression &&
+        (Info.CallStackDepth == 1 ||
----------------
xbolva00 wrote:
> !Info.CheckingPotentialConstantExpression
> 
> Basically just to silence test case:
> namespace std {
> constexpr bool is_constant_evaluated() noexcept {
>   return __builtin_is_constant_evaluated();
> }
> }
> 
> In real world code, this will not warn, since call's loc is in system header. Leave it?
The check seems right to me.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10597
+        (Info.CallStackDepth == 1 ||
+         (Info.CallStackDepth == 2 && E->getNumArgs() == 0 &&
+          Callee->isInStdNamespace() && Callee->getIdentifier() &&
----------------
The number of arguments check is redundant; we know the builtin has no parameters.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10600
+          Callee->getIdentifier()->isStr("is_constant_evaluated")))) {
+      if (Info.EvalStatus.Diag)
+        Info.report((Info.CallStackDepth == 1) ? E->getExprLoc()
----------------
xbolva00 wrote:
> Without this condition, it warns 3 times for
> 
> if constexpr (std::is_constant_evaluated() == false) 
Please add a "// FIXME: Find a better way to avoid duplicated diagnostics." or similar. This is relying somewhat on the implementation details of callers of the constant evaluator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69518