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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Note I didn't have a good chance to re-review this, but wanted to give my feedback to the comments right away.



================
Comment at: clang/include/clang/AST/Stmt.h:166
 
+    /// True if this if statement is a if consteval statement.
+    unsigned IsConsteval : 1;
----------------
cor3ntin wrote:
> 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?
My best suggestion is somewhere in include/Basic.

We don't have a great file I think to fit it, but we DO have a ton where we've created files for smal lthings (see ExceptionSpecificationType.h, Linkage.h, or Lambda.h).

Looking through, there _IS_ a similar enum in Specifiers.h (perhaps the best place for this) that might work well, ConstexprSpecKind might actually just be what we want and could replace the other enum you created later.


================
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++) {
----------------
cor3ntin wrote:
> 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
> 
I more questioned due to the lack of the `*`, I see it is the reverse-iterator now, so i don't think that makes it a pointer type, so I don't think it is valuable here.  @aaron.ballman is our 'auto nitpicker', so I'll let him to suggest the right answer.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1367
+  }
+  if(IsConsteval)
+  {
----------------
So I still meant here putting it inside the 'if (Tok.is(tok::kw_consteval)' bit, I have a slight preference to match the rest of these sorts of warnings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482