erichkeane 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;
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > 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.
> > 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.
>
> Unfortunately I have stronger feelings here. We need the bit-fields to all
> have the same type, otherwise there's problems with layout in MSVC (also, we
> often get odd diagnostic behavior we have to come back and fix later, as in
> https://reviews.llvm.org/rGaf3a4e97d8627a32606ed32001583fe08f15b928). That's
> why we use `unsigned` for all our bit-fields.
Ah shucks! Thanks for that Aaron! I still like having IfStatementKind for the
interface, but it seems like we'll have to static-cast in/out here.
================
Comment at: clang/include/clang/AST/Stmt.h:2081-2095
+ bool isConsteval() const {
+ return IfStmtBits.Kind == IfStatementKind::Consteval;
+ }
+ bool isConstexpr() const {
+ return IfStmtBits.Kind == IfStatementKind::Constexpr;
+ }
+
----------------
aaron.ballman wrote:
> How do folks feel about this? My concern is that `if ! consteval (whatever)`
> is still a consteval if statement, and we care "is this a consteval if
> statement" more often than we care which form it is, so it's more helpful
> that `isConsteval()` returns true for both forms. For the times when we care
> about negated vs nonnegated, we can use the more explicit functions (which
> have better parity with their names).
I DO think I like that, the `isConstevalorNegatedConsteval` was a bit of
heartburn running through this, but I couldn't come up with a better spelling.
I think this _IS_ that better spelling.
================
Comment at: clang/include/clang/Basic/Specifiers.h:45
+ Consteval,
+ ConstevalNegated
+ };
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > Reads nicer to me maybe? IDK, feel free to ignore.
> Alternatively, `ConstevalNonnegated` and `ConstevalNegated` to make it more
> clear that they're distinguishable from one another. (I don't feel strongly
> about any particular opinion here.)
Mixed with your other suggestion on changing the functions, I think this ends
up being a good suggestion.
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
