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