[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/Parse/ParseExprCXX.cpp:2907-2909
+    const Token Next = GetLookAheadToken(2);
+    const Token After = GetLookAheadToken(3);
+    const Token Last = GetLookAheadToken(4);
----------------
Please don't request the additional lookahead tokens until you've checked `Next`. (Maybe factor out some parts of the check into a lambda so you can return early?)


================
Comment at: lib/Parse/ParseExprCXX.cpp:2919
+      // to disambiguate it from 'delete[]'.
+      ExprResult Lambda = TryParseLambdaExpression();
+      if (!Lambda.isInvalid()) {
----------------
`TryParseLambdaExpression` will always commit to parsing as a //lambda-expression//, because the first two tokens are `[]`. Since you've already determined that we definitely have a *lambda-expression* here, you may as well just call `ParseLambdaExpression` directly.


================
Comment at: lib/Parse/ParseExprCXX.cpp:2925
+        
+        SourceLocation BeforeBracket = StartLoc.getLocWithOffset(-1);
+        Diag(BeforeBracket, diag::note_lambda_after_delete)
----------------
This offset of -1 isn't right: when we have a fix-it pointing at a character, insertions are inserted before that character. Given `delete[]{}`, this would insert the `(` between `delet` and `e[]`, rather than between `delete` and `[]`.


================
Comment at: lib/Parse/ParseExprCXX.cpp:2929
+            << FixItHint::CreateInsertion(
+                   Lambda.get()->getLocEnd().getLocWithOffset(1), ")");
+
----------------
Instead of `getLocWithOffset` here, use `Lexer::getLocForEndOfToken`; `getLocWIthOffset(1)` will return the wrong location if the closing brace token's length is greater than 1 (which can happen in the presence of escaped newlines).


================
Comment at: lib/Parse/ParseExprCXX.cpp:2935-2936
+                                      Lambda.get());
+      }
+      else
+        return ExprError();
----------------
Our coding style puts the `else` on the same line as the `}`.

However, we also generally don't like using `else` when the `if` block returns. And in this case I'd reverse the sense of the earlier `if`:

```
if (Lambda.isInvalid())
  return ExprError();
```


https://reviews.llvm.org/D36357