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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:195
+  if (IS->isNegatedConsteval())
+    return IS->getElse();
+
----------------
aaron.ballman wrote:
> This probably signals that we're missing some test coverage, but this is the first I've heard of this file so I'm not clear on *how* to test it!
Ouch, nasty! I added tests


================
Comment at: clang/lib/AST/StmtPrinter.cpp:242
+    if(If->isNegatedConsteval())
+        OS << "!";
+    OS << "consteval";
----------------
erichkeane wrote:
> 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 
I could have :)
Also, it's easier to see that the `!` appertain to `consteval`


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1449
+        Sema::ExpressionEvaluationContextRecord::EK_Other, ShouldEnter);
     ThenStmt = ParseStatement(&InnerStatementTrailingElseLoc);
   }
----------------
aaron.ballman wrote:
> This looks incorrect for a consteval if -- that requires parsing a compound statement explicitly, not just any statement.
See a little bit below


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1520-1525
+  if (IsConsteval) {
+    if (!isa_and_nonnull<class CompoundStmt>(ThenStmt.get())) {
+      Diag(ConstevalLoc, diag::err_expected_after) << "consteval"
+                                                   << "{";
+      return StmtError();
+    }
----------------
aaron.ballman wrote:
> Oh. I see where the diagnostic is happening. I think it's unclean to parse a statement and then diagnose it later as being the wrong kind of statement -- why not parse the correct kind of statement initially?
I think the code reads better that way - we need to parse any kind of statement in other cases. In any case the work is about the same


================
Comment at: clang/lib/Sema/SemaStmt.cpp:929
+      Diags.Report(IfLoc, diag::warn_if_consteval_always_true)
+          << (Immediate ? 1 : 0);
+    }
----------------
aaron.ballman wrote:
> Conversions just work this way (http://eel.is/c++draft/conv.integral#2).
It just seemed more explicit


================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2756-2758
+          /* HasElse=*/Record[ASTStmtReader::NumStmtFields],
+          /* HasVar=*/Record[ASTStmtReader::NumStmtFields + 1],
+          /* HasInit=*/Record[ASTStmtReader::NumStmtFields + 2]);
----------------
aaron.ballman wrote:
> Can you explain these changes? I'm not certain why they were needed.
I could revert them back actually - I'm just storing the constexpr info after, which seems simpler


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482