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