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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:5712
+    // predicate expression.
+    return (int)isExprPredicate();
+  }
----------------
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)?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:19851
+    bool IsExpr = GSE->isExprPredicate();
+    if (IsExpr)
+      ExOrTy = GSE->getControllingExpr();
----------------
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?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149904