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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
