[PATCH] D149904: Generic selection expressions that accept a type operand

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 10 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:5712
+    // predicate expression.
+    return (int)isExprPredicate();
+  }
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > Should these have asserts also?  Wouldn't saying this is index '0' be incorrect here if t his is the typePredicate?
> > Nope, these should not assert -- the selection is required to have at least one association (and an association is a pair of type and expr), so this is getting you the offset to the first association (either the expr or the type).
> OH! I see, there are 2 separate lists here, 1 for types, 1 for expressions, is that it?  And you're storing the controlling expression/type in the 1st one (if it exists)?
Correct! Two *almost* parallel lists where the first entry in either (but not both) list is potentially the controlling operand.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:19851
+    bool IsExpr = GSE->isExprPredicate();
+    if (IsExpr)
+      ExOrTy = GSE->getControllingExpr();
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > I find myself wondering if a `getControllingOperand` here is valuable?
> > This pattern only comes up once, so I don't think it's worth it, but it's certainly easy enough to add if you disagree.
> Yep, I think I agree, after seeing the rest of the review, I don't see as much value. 
> 
> On this re-read, I kind of which we instead had 2 separate 'create' functions here, rather than going through void*, so that this becomes:
> 
> ```
> if (IsExpr)
>   return AnyChanged ? S.CreateGenericSelectionExpr(...., GSE->getControllingExpr(), ...) : ExprEmpty();
> 
>   return AnyChanged ? S.CreateGenericSelectionExpr(...., GSE->getControllingType(), ...) : ExprEmpty();
> ```
> 
> The `void*` version gives me the jeebies (and is perhaps a 'worse' interface?  Particularly for libclang folks?).
That makes this one use better but at the expense of making that create function rather awkward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149904