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

[Erich Keane via Phabricator via cfe-commits]

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