[PATCH] D110482: [clang] Implement if consteval (P1938)

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So I have some heartburn about `IsNegatedConsteval`.  A part of me wants to suggest ditching the `ConstexprKind` and creating a new scoped-enum `IfStmtKind` (perhaps in the same file) that has just `None,Constexpr,Consteval,NotConsteval`.  Its already a bit hinky that we have a value of `ConstexprKind` that we don't use, so I'm leaning toward it.  Other reviewers: WDYT?

Otherwise, I think clang-format REALLY needs a run here, and the codegen test can be fleshed out a bit more.  Otherwise, this seems right to me.



================
Comment at: clang/include/clang/AST/Stmt.h:1971
                         SourceLocation EL = SourceLocation(),
-                        Stmt *Else = nullptr);
+                        Stmt *Else = nullptr, bool IfConstevalIsNegated = false);
 
----------------
Not a fan of making this 'IfConstevalIsNegated' a default here... it means it has to be at the end, where it doesn't particularly make sense.  Plus, you have to change all the calls to this function ANYWAY, right?


================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:1493
+  attributeOnlyIfTrue("isConsteval", IS->isConsteval());
+  attributeOnlyIfTrue("constevalIsNegated", IS->isConsteval() && IS->isNegatedConsteval());
 }
----------------
Is the first condition necessary here?  I would think isNegatedConsteval should NEVER be true if isConsteval?


================
Comment at: clang/lib/AST/StmtPrinter.cpp:242
+    if(If->isNegatedConsteval())
+        OS << "!";
+    OS << "consteval";
----------------
You had a choice here.... You COULD have blessed the 'c++ operator names', but chose not to.  I guess we all have to choose teams:D 


================
Comment at: clang/test/CodeGenCXX/cxx2b-consteval-if.cpp:1
+// RUN: %clang_cc1 -std=c++2b %s -emit-llvm -o - | FileCheck %s --implicit-check-not=should_not_be_used
+
----------------
I tend to like the 'CHECK' lines being inline with the code, it makes it easier to follow in these cases.  Additionally, I think the check-lines should be more specific (that they are actually checking for 'call' instructions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482