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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits