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

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

rsmith added a comment.

This looks good, with some minor changes. Please add more test coverage, though, specifically:

- test all four forms of lambda that we recognize after `delete`
- add a test that the FixItHint is correct (maybe in test/FixIt/fixit-cxx0x.cpp, which checks that applying the fixes results in working code)



================
Comment at: lib/Parse/ParseExprCXX.cpp:2956
+         GetLookAheadToken(3).isOneOf(tok::r_paren, tok::identifier) &&
+         GetLookAheadToken(4).is(tok::identifier))) {
+      SourceLocation RightBracketLock = NextToken().getLocation();
----------------
This doesn't seem quite right now. `[]()` is not recognized as a lambda unless it's followed by an identifier. I think we want (and sorry if this is reverting to what you had before switching to `isOneOf`):

```
    if (Next.isOneOf(tok::l_brace, tok::less) ||
        (Next.is(tok::l_paren) &&
         (GetLookAheadToken(3).is(tok::r_paren) ||
          (GetLookAheadToken(3).is(tok::identifier) && GetLookAheadToken(4).is(tok::r_paren))))) {
```
That is, we're matching:
* `[]{`
* `[]<`
* `[]()`
* `[](identifier identifier`



================
Comment at: lib/Parse/ParseExprCXX.cpp:2969
+          << FixItHint::CreateInsertion(
+                 Lexer::getLocForEndOfToken(Lambda.get()->getLocEnd(), 1,
+                                            Actions.getSourceManager(),
----------------
The `1` here should be a `0`. Given
```
delete []{return new int;}();
```
this will insert the `)` after the final `;` instead of before it.


================
Comment at: lib/Parse/ParseExprCXX.cpp:2978
+                                    Lambda.get());
+    } else {
+      ArrayDelete = true;
----------------
No need for an `else` here, since the `if` block terminates by `return`.


Repository:
  rC Clang

https://reviews.llvm.org/D36357