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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/AST/Stmt.h:166
 
+    /// True if this if statement is a if consteval statement.
+    unsigned IsConsteval : 1;
----------------
Not sure how others feel here, but for me, I kinda hate that we're using 'unsigned' bitfields for all of this, particularly because these two are mutually exclusive.  I'd prefer (though listen to others here first) if the type of this was something more like:

IfEvalKind EvalKind : 2; // where IfEvalKind is enum class IfEvalKind {None, Constexpr, Consteval};


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5942
   "jump bypasses initialization of VLA type alias">;
 def note_protected_by_constexpr_if : Note<
   "jump enters controlled statement of constexpr if">;
----------------
Oh boy.... this section of 'notes' seem like they deserve some level of merging with a %select.  I'd not want anything other than the `jump enters controlled statemennt of `  variants for this patch though.


================
Comment at: clang/include/clang/Sema/Sema.h:1320
+
+    bool isImmediate() const {
+      return Context == ExpressionEvaluationContext::ImmediateFunctionContext;
----------------
I get this NOW that I'm looking at it, but `isImmediate` made me go to assembly-immediate, though perhaps this is only misleading to me.  I might bikeshed this to `isImmediatelyEvaluated` or something.
Anyone else, WDYT?  


================
Comment at: clang/include/clang/Sema/Sema.h:4723
   class ConditionResult;
-  StmtResult ActOnIfStmt(SourceLocation IfLoc, bool IsConstexpr,
+  enum IfKind {
+    IfKind_Default,
----------------
can this be a scoped enum?

Also, perhaps pulled into the AST in a way that it can be reused for the IfStmt state.


================
Comment at: clang/include/clang/Sema/Sema.h:9137
+           "Must be in an expression evaluation context");
+    for (auto it = ExprEvalContexts.rbegin(); it != ExprEvalContexts.rend();
+         it++) {
----------------
What is our iterator type?  I THINK at minimum require this to be `auto *`


================
Comment at: clang/lib/AST/Stmt.cpp:927
 
+  assert((!IsConsteval && !IsConstexpr) || IsConsteval != IsConstexpr);
+
----------------
Oh dear... I'm not quite sure I want this as a scoped enum.


================
Comment at: clang/lib/AST/StmtPrinter.cpp:246
+      OS << "else";
+      PrintStmt(If->getThen());
+      OS << NL;
----------------
should this be `If->getElse`?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1363
+    if (Tok.is(tok::kw_consteval)) {
+      Diag(Tok, getLangOpts().CPlusPlus17 ? diag::warn_cxx14_compat_constexpr_if
+                                          : diag::ext_constexpr_if);
----------------
Is this diag here right?  We're in the `kw_consteval` branch, but it seems we're warning about constexpr-if?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1382
 
+  if (IsConsteval) {
+    Diag(Tok, getLangOpts().CPlusPlus2b ? diag::warn_cxx20_compat_consteval_if
----------------
Why is this here instead of with the `kw_consteval` handling?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:874
 
   Expr *CondExpr = Cond.get().second;
   // Only call the CommaVisitor when not C89 due to differences in scope flags.
----------------
perhaps an assert here: `assert((CondExpr || Kind == IfKind_Consteval) && "....")`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482