sammccall added inline comments.
================ Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5258 // Peel off the ParenListExpr by chosing the last one, as they don't have a // predefined type. ---------------- I think we should merge the two comments, something like... ``` /// If LHS is ParenListExpr, assume a chain of comma operators and pick the last expr. /// We expect other ParenListExprs to be resolved to e.g. constructor calls before here. /// (So the ParenListExpr should be nonempty, but check just in case) ``` ================ Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5260 // predefined type. - if (auto *PLE = llvm::dyn_cast<ParenListExpr>(Base)) + if (auto *PLE = llvm::dyn_cast<ParenListExpr>(Base)) { + // FIXME: Get rid of this check once we are sure the ParenListExpr in here ---------------- now this is getting a little more complicated, we could pull out a function Expr* unwrapParenList(Expr*) and avoid duplicating code & comments, but up to you. ================ Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5261 + if (auto *PLE = llvm::dyn_cast<ParenListExpr>(Base)) { + // FIXME: Get rid of this check once we are sure the ParenListExpr in here + // cannot be empty. This is assumed to ultimately be a chain of comma ---------------- I'm not actually sure we should fix this. Nonempty is a subtle invariant that could also be violated by e.g. broken code. And the check is cheap. ================ Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5272 + if (PLE->getNumExprs() == 0) + return; OtherOpBase = PLE->getExpr(PLE->getNumExprs() - 1); ---------------- I think you want OtherOpBase = null here, not return Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96950/new/ https://reviews.llvm.org/D96950 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits