[PATCH] D96950: [clang][CodeComplete] Ensure there are no crashes when completing with ParenListExprs as LHS

[Sam McCall via Phabricator via cfe-commits]

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