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