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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Thanks for working on this!
I left some comments, i hope it helps!



================
Comment at: clang/include/clang/Sema/Sema.h:1277
+    /// as PotentiallyEvaluated.
+    RuntimeEvaluated
   };
----------------
I think I'd prefer a boolean for that, `ExpressionEvaluationContext` somewhat matches standard definitions.
I think we might mostly be able to reuse PotentiallyEvaluatedIfUsed | PotentiallyEvaluated.

RuntimeEvaluated is anything that does not have a parent context that is constant evaluated/immediate/unevaluated. Maybe you could try to make a function for that?


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3208-3211
 ExprResult Parser::ParseCXXMemberInitializer(Decl *D, bool IsFunction,
+                                             bool IsConstexpr,
                                              SourceLocation &EqualLoc) {
   assert(Tok.isOneOf(tok::equal, tok::l_brace) &&
----------------
I think I'd prefer casting D to a VarDecl rather than adding a parameter


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3214-3221
   EnterExpressionEvaluationContext Context(
       Actions,
       isa_and_present<FieldDecl>(D)
           ? Sema::ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed
           : Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
       D);
+  Actions.ExprEvalContexts.back().InConstantEvaluated =
----------------
Maybe we should instead say that a constexpr FieldDecl is ConstantEvaluated ?
`InConstantEvaluated` in general seems redundant


================
Comment at: clang/lib/Parse/ParseExpr.cpp:240
       Actions, Sema::ExpressionEvaluationContext::Unevaluated);
+  Actions.ExprEvalContexts.back().InConstantEvaluated = true;
   ExprResult LHS(ParseCastExpression(AnyCastExpr));
----------------
I'd rather either get rid of `InConstantEvaluated`, or at least consider UnevaluatedContext as ConstantEvaluated everywhere (ie by changing DiagnoseTautologicalIsConstantEvaluated)


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1513-1516
+    EnterExpressionEvaluationContext Consteval(
+        Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+        /*LambdaContextDecl=*/nullptr,
+        Sema::ExpressionEvaluationContextRecord::EK_Other, IsConstexpr);
----------------
This seems wrong, the condition of a if statement is not constant evaluated.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:15241-15252
+  if (!isLambdaCallOperator(FD)) {
     // [expr.const]/p14.1
     // An expression or conversion is in an immediate function context if it is
     // potentially evaluated and either: its innermost enclosing non-block scope
     // is a function parameter scope of an immediate function.
+
     PushExpressionEvaluationContext(
----------------
There are a bunch of white spaces only changes there


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17962-18006
-/// Determine whether the given declaration is a global variable or
-/// static data member.
-static bool isNonlocalVariable(const Decl *D) {
-  if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(D))
-    return Var->hasGlobalStorage();
-
-  return false;
----------------
Can you explain the changes to this file? they seems to affect test cases unrelated to the goal of this PR


================
Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:1964
   void f() {
-    constexpr int &n = n; // expected-error {{constant expression}} expected-note {{use of reference outside its lifetime}} expected-warning {{not yet bound to a value}}
     constexpr int m = m; // expected-error {{constant expression}} expected-note {{read of object outside its lifetime}}
----------------
This change does not seem correct


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155064