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

[Erich Keane via Phabricator via cfe-commits]

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