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