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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin 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;
----------------
erichkeane wrote:
> 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};
I was not quite sure where to put an enum I could reuse in different place.
But I think I'd agree with you otherwise.
Any suggestion for where to put it?


================
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++) {
----------------
erichkeane wrote:
> What is our iterator type?  I THINK at minimum require this to be `auto *`
The iterator type is ungodly. Likely `SmallVector<ExpressionEvaluationContextRecord, 8>::reverse_iterator`. I think auto here is more readable but I can change if you want to



================
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);
----------------
erichkeane wrote:
> Is this diag here right?  We're in the `kw_consteval` branch, but it seems we're warning about constexpr-if?
Yup, forgot to remove that


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1382
 
+  if (IsConsteval) {
+    Diag(Tok, getLangOpts().CPlusPlus2b ? diag::warn_cxx20_compat_consteval_if
----------------
erichkeane wrote:
> Why is this here instead of with the `kw_consteval` handling?
To not have a warning when there is an error. Maybe we don't care?
In any case, I've cleanup that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482