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

Fri May 5 07:42:42 PDT 2023

aaron.ballman added a comment.

In D149904#4321144 <https://reviews.llvm.org/D149904#4321144>, @cor3ntin wrote:

> I observe there is no documentation or release notes yet :)

Those are coming up, I wanted to see if the RFC made any substantive changes to the design before doing the documentation for the extension.

================
Comment at: clang/lib/Sema/SemaExpr.cpp:19851
+    bool IsExpr = GSE->isExprPredicate();
+    if (IsExpr)
+      ExOrTy = GSE->getControllingExpr();
----------------
aaron.ballman wrote:
> 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.
I took another run at this and decided to put a FIXME comment in the header file. I agree that the interface is a bit awkward, but it's also pretty hard to make work in `ActOnGenericSelectionExpr` due to it taking an `Expr *` or a `ParsedType` (currently it takes a `void *` of the parsed type's opaque value).

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

https://reviews.llvm.org/D149904