erichkeane added a comment.
A handful of 'nits' (at best), and a wish for the `if consteval` vs `consteval
if` to be consistent, but otherwise LGTM. Hopefully @aaron.ballman can come
and do a final pass.
================
Comment at: clang/include/clang/AST/Stmt.h:164
- /// True if this if statement is a constexpr if.
- unsigned IsConstexpr : 1;
+ /// Whether this is an if constexpr if or a consteval if or neither.
+ IfStatementKind Kind : 3;
----------------
================
Comment at: clang/include/clang/AST/Stmt.h:2122
child_range children() {
- return child_range(getTrailingObjects<Stmt *>(),
+ // We always store a condition, but there is none for if consteval
+ // statements, so skip it.
----------------
Not sure if `consteval if` follows the same naming convention as `constexpr if`?
================
Comment at: clang/include/clang/AST/Stmt.h:2131
const_child_range children() const {
- return const_child_range(getTrailingObjects<Stmt *>(),
- getTrailingObjects<Stmt *>() +
- numTrailingObjects(OverloadToken<Stmt *>()));
+ // We always store a condition, but there is none for if consteval
+ // statements, so skip it.
----------------
Same change here.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1505
+def warn_if_consteval_always_true : Warning<
+ "if consteval is always true in an %select{unevaluated|immediate}0
context">, InGroup<DiagGroup<"redundant-if-consteval">>;
----------------
This seems like it needs a newline somewhere/clang-formatting.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1505
+def warn_if_consteval_always_true : Warning<
+ "if consteval is always true in an %select{unevaluated|immediate}0
context">, InGroup<DiagGroup<"redundant-if-consteval">>;
----------------
erichkeane wrote:
> This seems like it needs a newline somewhere/clang-formatting.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1506
+def warn_if_consteval_always_true : Warning<
+ "if consteval is always true in an %select{unevaluated|immediate}0
context">, InGroup<DiagGroup<"redundant-if-consteval">>;
+
----------------
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5945
+def note_protected_by_if_consteval : Note<
+ "jump enters controlled statement of if consteval">;
def note_protected_by_if_available : Note<
----------------
I see an inconsistency in a couple of places here between `if consteval` or
`consteval if`. I THINK it is the latter, so I'm suggesting changes for that.
I see the DiagnosticParseKinds seem to use the `consteval if` spelling, so I
feel like perhaps I'm not wrong?
================
Comment at: clang/include/clang/Basic/Specifiers.h:32
/// Define the kind of constexpr specifier.
- enum class ConstexprSpecKind { Unspecified, Constexpr, Consteval, Constinit
};
+ enum class ConstexprSpecKind : unsigned {
+ Unspecified,
----------------
I'd prefer just reverting the changes to ConstexprSpecKind to the 'before',
since you're not using it anymore.
================
Comment at: clang/include/clang/Basic/Specifiers.h:40
+ /// In an if statement, this denotes whether the the statement is
+ /// an if constexpr or if consteval statement.
+ enum class IfStatementKind : unsigned {
----------------
================
Comment at: clang/include/clang/Basic/Specifiers.h:45
+ Consteval,
+ ConstevalNegated
+ };
----------------
Reads nicer to me maybe? IDK, feel free to ignore.
================
Comment at: clang/include/clang/Sema/Sema.h:1223
+ /// occurs in an immediate function context - either a consteval function
+ /// or an 'if consteval' function.
+ ImmediateFunctionContext,
----------------
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
