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

[Nicolas Lesser via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL361119: Added a better diagnostic when using the delete 
operator with lambdas (authored by Rakete, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D36357?vs=199566=200180#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357

Files:
  cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
  cfe/trunk/lib/Parse/ParseExprCXX.cpp
  cfe/trunk/test/FixIt/fixit-cxx0x.cpp
  cfe/trunk/test/Parser/cxx0x-lambda-expressions.cpp
  cfe/trunk/test/SemaCXX/new-delete-0x.cpp

Index: cfe/trunk/lib/Parse/ParseExprCXX.cpp
===
--- cfe/trunk/lib/Parse/ParseExprCXX.cpp
+++ cfe/trunk/lib/Parse/ParseExprCXX.cpp
@@ -3034,8 +3034,58 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
+
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+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::identifier) {
+  TentativeParsingAction TPA(*this);
+  SourceLocation LSquareLoc = Tok.getLocation();
+  SourceLocation RSquareLoc = NextToken().getLocation();
+
+  // SkipUntil can't skip pairs of ; don't emit a FixIt in this
+  // case.
+  SkipUntil({tok::l_brace, tok::less}, StopBeforeMatch);
+  SourceLocation RBraceLoc;
+  bool EmitFixIt = false;
+  if (TryConsumeToken(tok::l_brace)) {
+SkipUntil(tok::r_brace, StopBeforeMatch);
+RBraceLoc = Tok.getLocation();
+EmitFixIt = true;
+  }
+
+  TPA.Revert();
+
+  if (EmitFixIt)
+Diag(Start, diag::err_lambda_after_delete)
+<< SourceRange(Start, RSquareLoc)
+<< FixItHint::CreateInsertion(LSquareLoc, "(")
+<< FixItHint::CreateInsertion(
+   Lexer::getLocForEndOfToken(
+   RBraceLoc, 0, Actions.getSourceManager(), getLangOpts()),
+   ")");
+  else
+Diag(Start, diag::err_lambda_after_delete)
+<< SourceRange(Start, RSquareLoc);
+
+  // Warn that the non-capturing lambda isn't surrounded by parentheses
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  if (Lambda.isInvalid())
+return ExprError();
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+}
+
 ArrayDelete = true;
 BalancedDelimiterTracker T(*this, tok::l_square);
 
Index: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
@@ -109,6 +109,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'; add parentheses to treat this as a lambda-expression">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">,
Index: cfe/trunk/test/SemaCXX/new-delete-0x.cpp
===
--- cfe/trunk/test/SemaCXX/new-delete-0x.cpp
+++ cfe/trunk/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,5 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
Index: cfe/trunk/test/FixIt/fixit-cxx0x.cpp
===
--- cfe/trunk/test/FixIt/fixit-cxx0x.cpp
+++ cfe/trunk/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // expected-error{{lambda requires '()' before return 

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

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

rsmith accepted this revision.
rsmith added a comment.

LGTM, thank you! :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 199565.
Rakete added a comment.

- Fix crash with invalid postfix expr
- Don't emit FixIt when using <


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++2a %s
 
 enum E { e };
 
@@ -43,31 +44,57 @@
 int a4[1] = {[] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'const int *'}}
 int a5[3] = { []{return 0;}() };
 int a6[1] = {[this] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'C *'}}
-int a7[1] = {[d(0)] { return d; } ()}; // expected-warning{{extension}}
-int a8[1] = {[d = 0] { return d; } ()}; // expected-warning{{extension}}
+int a7[1] = {[d(0)] { return d; } ()};
+int a8[1] = {[d = 0] { return d; } ()};
+int a10[1] = {[id(0)] { return id; } ()};
+#if __cplusplus <= 201103L
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+#endif
 int a9[1] = {[d = 0] = 1}; // expected-error{{is not an integral constant expression}}
-int a10[1] = {[id(0)] { return id; } ()}; // expected-warning{{extension}}
+#if __cplusplus >= 201402L
+// expected-note@-2{{constant expression cannot modify an object that is visible outside that expression}}
+#endif
 int a11[1] = {[id(0)] = 1};
   }
 
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#if __cplusplus > 201703L
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#endif
   }
 
   // We support init-captures in C++11 as an extension.
   int z;
   void init_capture() {
-[n(0)] () mutable -> int { return ++n; }; // expected-warning{{extension}}
-[n{0}] { return; }; // expected-warning{{extension}}
-[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}} expected-warning{{extension}}
-[n = {0}] { return; }; // expected-error {{}} expected-warning{{extension}}
-[a([ = z]{})](){}; // expected-warning 2{{extension}}
+[n(0)] () mutable -> int { return ++n; };
+[n{0}] { return; };
+[a([ = z]{})](){};
+[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}}
+[n = {0}] { return; }; // expected-error {{}}
+#if __cplusplus <= 201103L
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+#endif
 
 int x = 4;
-auto y = [ = x, x = x + 1]() -> int { // expected-warning 2{{extension}}
+auto y = [ = x, x = x + 1]() -> int {
+#if __cplusplus <= 201103L
+  // expected-warning@-2{{extension}}
+  // expected-warning@-3{{extension}}
+#endif
   r += 2;
   return x + 2;
 } ();
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 199566.
Rakete added a comment.

- Use TryConsumeToken


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++2a %s
 
 enum E { e };
 
@@ -43,31 +44,57 @@
 int a4[1] = {[] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'const int *'}}
 int a5[3] = { []{return 0;}() };
 int a6[1] = {[this] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'C *'}}
-int a7[1] = {[d(0)] { return d; } ()}; // expected-warning{{extension}}
-int a8[1] = {[d = 0] { return d; } ()}; // expected-warning{{extension}}
+int a7[1] = {[d(0)] { return d; } ()};
+int a8[1] = {[d = 0] { return d; } ()};
+int a10[1] = {[id(0)] { return id; } ()};
+#if __cplusplus <= 201103L
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+#endif
 int a9[1] = {[d = 0] = 1}; // expected-error{{is not an integral constant expression}}
-int a10[1] = {[id(0)] { return id; } ()}; // expected-warning{{extension}}
+#if __cplusplus >= 201402L
+// expected-note@-2{{constant expression cannot modify an object that is visible outside that expression}}
+#endif
 int a11[1] = {[id(0)] = 1};
   }
 
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#if __cplusplus > 201703L
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#endif
   }
 
   // We support init-captures in C++11 as an extension.
   int z;
   void init_capture() {
-[n(0)] () mutable -> int { return ++n; }; // expected-warning{{extension}}
-[n{0}] { return; }; // expected-warning{{extension}}
-[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}} expected-warning{{extension}}
-[n = {0}] { return; }; // expected-error {{}} expected-warning{{extension}}
-[a([ = z]{})](){}; // expected-warning 2{{extension}}
+[n(0)] () mutable -> int { return ++n; };
+[n{0}] { return; };
+[a([ = z]{})](){};
+[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}}
+[n = {0}] { return; }; // expected-error {{}}
+#if __cplusplus <= 201103L
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+#endif
 
 int x = 4;
-auto y = [ = x, x = x + 1]() -> int { // expected-warning 2{{extension}}
+auto y = [ = x, x = x + 1]() -> int {
+#if __cplusplus <= 201103L
+  // expected-warning@-2{{extension}}
+  // expected-warning@-3{{extension}}
+#endif
   r += 2;
   return x + 2;
 } ();
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // 

[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.

In D36357#1501999 , @Rakete wrote:

> In D36357#1501961 , @rsmith wrote:
>
> > In D36357#1500949 , @Rakete 
> > wrote:
> >
> > > How should I do this? Do I just skip to the next `}`, or also take into 
> > > account  any additional scopes? Also does this mean that I skip and then 
> > > revert, because that seems pretty expensive?
> >
> >
> > It would be a little expensive, yes, but we'd only be doing it on codepaths 
> > where we're producing an error -- for an ill-formed program, it's OK to 
> > take more time in order to produce a better diagnostic. Skipping to the 
> > next `}` won't work, because `SkipUntil` will skip over pairs of 
> > brace-balanced tokens (so you'll skip past the `}` you're looking for), but 
> > skipping until the next `{` and then skipping to the `}` after it should 
> > work.
>
>
> Hmm wouldn't this interact badly with `{}` in initializers?
>
>   [] {};
>   [](int = {0}) {};
>


Ouch. The first of these two seems like it won't work, since `SkipUntil` has no 
idea the angles are brackets. =( Getting this right is ... really hairy. Eg:

  []{} // end of lambda here?
  >(){} // or here?

(The answer depends on whether `a` is a template.) And the same problem shows 
up in the //trailing-return-type// and the //requires-clause//.

How about this: `SkipUntil` an `{` or `<`. If you find a `<` first, don't 
attach a `FixItHint` to the diagnostic since we can't easily tell where to 
suggest putting the `)`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 199528.
Rakete marked an inline comment as done.
Rakete added a comment.

Nevermind, seems to be working fine even with.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++2a %s
 
 enum E { e };
 
@@ -43,31 +44,57 @@
 int a4[1] = {[] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'const int *'}}
 int a5[3] = { []{return 0;}() };
 int a6[1] = {[this] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'C *'}}
-int a7[1] = {[d(0)] { return d; } ()}; // expected-warning{{extension}}
-int a8[1] = {[d = 0] { return d; } ()}; // expected-warning{{extension}}
+int a7[1] = {[d(0)] { return d; } ()};
+int a8[1] = {[d = 0] { return d; } ()};
+int a10[1] = {[id(0)] { return id; } ()};
+#if __cplusplus <= 201103L
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+#endif
 int a9[1] = {[d = 0] = 1}; // expected-error{{is not an integral constant expression}}
-int a10[1] = {[id(0)] { return id; } ()}; // expected-warning{{extension}}
+#if __cplusplus >= 201402L
+// expected-note@-2{{constant expression cannot modify an object that is visible outside that expression}}
+#endif
 int a11[1] = {[id(0)] = 1};
   }
 
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#if __cplusplus > 201703L
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#endif
   }
 
   // We support init-captures in C++11 as an extension.
   int z;
   void init_capture() {
-[n(0)] () mutable -> int { return ++n; }; // expected-warning{{extension}}
-[n{0}] { return; }; // expected-warning{{extension}}
-[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}} expected-warning{{extension}}
-[n = {0}] { return; }; // expected-error {{}} expected-warning{{extension}}
-[a([ = z]{})](){}; // expected-warning 2{{extension}}
+[n(0)] () mutable -> int { return ++n; };
+[n{0}] { return; };
+[a([ = z]{})](){};
+[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}}
+[n = {0}] { return; }; // expected-error {{}}
+#if __cplusplus <= 201103L
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+#endif
 
 int x = 4;
-auto y = [ = x, x = x + 1]() -> int { // expected-warning 2{{extension}}
+auto y = [ = x, x = x + 1]() -> int {
+#if __cplusplus <= 201103L
+  // expected-warning@-2{{extension}}
+  // expected-warning@-3{{extension}}
+#endif
   r += 2;
   return x + 2;
 } ();
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete marked 2 inline comments as done.
Rakete added a comment.

In D36357#1501961 , @rsmith wrote:

> In D36357#1500949 , @Rakete 
> wrote:
>
> > How should I do this? Do I just skip to the next `}`, or also take into 
> > account  any additional scopes? Also does this mean that I skip and then 
> > revert, because that seems pretty expensive?
>
>
> It would be a little expensive, yes, but we'd only be doing it on codepaths 
> where we're producing an error -- for an ill-formed program, it's OK to take 
> more time in order to produce a better diagnostic. Skipping to the next `}` 
> won't work, because `SkipUntil` will skip over pairs of brace-balanced tokens 
> (so you'll skip past the `}` you're looking for), but skipping until the next 
> `{` and then skipping to the `}` after it should work.


Hmm wouldn't this interact badly with `{}` in initializers?

  [] {};
  [](int = {0}) = {};






Comment at: clang/lib/Parse/ParseExprCXX.cpp:2996
+   GetLookAheadToken(4).is(tok::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis

rsmith wrote:
> rsmith wrote:
> > `RightBracketLock` -> `RSquareLoc`
> > 
> > (Our convention is to use `Loc` for "location" and to avoid the word 
> > "bracket" because it means different things in different English dialects 
> > -- usually `[]` in US English and usually `()` in UK English.)
> `Lock` -> `Loc`. There's no `k` in "location" =)
No idea how that k managed to sneak in :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

In D36357#1500949 , @Rakete wrote:

> How should I do this? Do I just skip to the next `}`, or also take into 
> account  any additional scopes? Also does this mean that I skip and then 
> revert, because that seems pretty expensive?


It would be a little expensive, yes, but we'd only be doing it on codepaths 
where we're producing an error -- for an ill-formed program, it's OK to take 
more time in order to produce a better diagnostic. Skipping to the next `}` 
won't work, because `SkipUntil` will skip over pairs of brace-balanced tokens 
(so you'll skip past the `}` you're looking for), but skipping until the next 
`{` and then skipping to the `}` after it should work.




Comment at: clang/lib/Parse/ParseExprCXX.cpp:2996
+   GetLookAheadToken(4).is(tok::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis

rsmith wrote:
> `RightBracketLock` -> `RSquareLoc`
> 
> (Our convention is to use `Loc` for "location" and to avoid the word 
> "bracket" because it means different things in different English dialects -- 
> usually `[]` in US English and usually `()` in UK English.)
`Lock` -> `Loc`. There's no `k` in "location" =)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 199380.
Rakete added a comment.

- Addressed review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++2a %s
 
 enum E { e };
 
@@ -53,8 +54,14 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#if __cplusplus > 201703L
+delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#endif
   }
 
   // We support init-captures in C++11 as an extension.
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // expected-error{{lambda requires '()' before return type}}
+
+  delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+  delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
 }
 
 #define bar "bar"
Index: clang/lib/Parse/ParseExprCXX.cpp
===
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -2984,8 +2984,38 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
+
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+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::identifier) {
+  SourceLocation RSquareLock = NextToken().getLocation();
+  // Warn that the non-capturing lambda isn't surrounded by parentheses
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getBeginLoc();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RSquareLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getEndLoc(), 0,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+}
+
 ArrayDelete = true;
 BalancedDelimiterTracker T(*this, tok::l_square);
 
Index: clang/include/clang/Basic/DiagnosticParseKinds.td

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete marked 3 inline comments as done.
Rakete added a comment.

> we could perform a tentative parse and skip to the } of the lambda.

How should I do this? Do I just skip to the next `}`, or also take into account 
 any additional scopes? Also does this mean that I skip and then revert, 
because that seems pretty expensive?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

In D36357#1491081 , @Rakete wrote:

> @rsmith One last question: The fixit diagnostic seems to be inconsistent with 
> the rest?
>
>   main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'
> delete[] { return new int; }
> ^~~~
>   ( )
>
>
> Shouldn't the `^~~` be starting at the `[]`?


Pointing the caret at the `[` might be clearer, but I think that's fine too.

In D36357#1491082 , @Rakete wrote:

> Is this also okay?
>
>   main.cpp:2:9: warning: lambda expressions are incompatible with C++98 
> [-Wc++98-compat]
> delete[] { return new int; }
>   ^
>   main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'; add 
> parentheses to treat this as a lambda-expression
> delete[] { return new int; }
> ^~~~
>   ( )
>


Hmm. It would be better to produce the error first, but that would make it more 
difficult to get the fixit hint right. I suppose in the case where we've 
already decided we're going to diagnose, we could perform a tentative parse and 
skip to the `}` of the lambda.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added a comment.

Is this also okay?

  main.cpp:2:9: warning: lambda expressions are incompatible with C++98 
[-Wc++98-compat]
delete[] { return new int; }
  ^
  main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'; add 
parentheses to treat this as a lambda-expression
delete[] { return new int; }
^~~~
  ( )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added a comment.

@rsmith One last question: The fixit diagnostic seems to be inconsistent with 
the rest?

  main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'
delete[] { return new int; }
^~~~
  ( )

Shouldn't the `^~~` be starting at the `[]`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks! Some minor nits, please feel free to commit once they're addressed.

In D36357#1177852 , @Rakete wrote:

> Note that clang doesn't support the fourth kind of lambda yet ([]<>), because 
> D36527  hasn't been merged yet, so I didn't 
> add a test case for that one.


We support that now, so please add a test for that :)




Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:109
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
 

Please add to this: "; add parentheses to treat this as a lambda-expression" or 
similar.



Comment at: clang/lib/Parse/ParseExprCXX.cpp:2996
+   GetLookAheadToken(4).is(tok::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis

`RightBracketLock` -> `RSquareLoc`

(Our convention is to use `Loc` for "location" and to avoid the word "bracket" 
because it means different things in different English dialects -- usually `[]` 
in US English and usually `()` in UK English.)



Comment at: clang/lib/Parse/ParseExprCXX.cpp:2997
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.

if -> that
parenthesis -> parentheses



Comment at: clang/lib/Parse/ParseExprCXX.cpp:3014
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,

Maybe we should do this before producing the diagnostic so that we can suggest 
inserting the `)` after the complete expression? (But I don't have a strong 
preference either way.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 198142.
Rakete added a comment.

friendly ping :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,8 +53,11 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
   }
 
   // We support init-captures in C++11 as an extension.
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // expected-error{{lambda requires '()' before return type}}
+
+  delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+  delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
 }
 
 #define bar "bar"
Index: clang/lib/Parse/ParseExprCXX.cpp
===
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -2984,8 +2984,38 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
+
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+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::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getBeginLoc();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RightBracketLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getEndLoc(), 0,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+}
+
 ArrayDelete = true;
 BalancedDelimiterTracker T(*this, tok::l_square);
 
Index: clang/include/clang/Basic/DiagnosticParseKinds.td
===
--- clang/include/clang/Basic/DiagnosticParseKinds.td
+++ clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -105,6 +105,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 190435.
Rakete added a comment.

Rebase + friendly ping :).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36357/new/

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,8 +53,11 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
   }
 
   // We support init-captures in C++11 as an extension.
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // expected-error{{lambda requires '()' before return type}}
+
+  delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+  delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
 }
 
 #define bar "bar"
Index: clang/lib/Parse/ParseExprCXX.cpp
===
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -2984,8 +2984,38 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
+
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+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::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getBeginLoc();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RightBracketLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getEndLoc(), 0,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+}
+
 ArrayDelete = true;
 BalancedDelimiterTracker T(*this, tok::l_square);
 
Index: clang/include/clang/Basic/DiagnosticParseKinds.td
===
--- clang/include/clang/Basic/DiagnosticParseKinds.td
+++ clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -105,6 +105,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 171421.
Rakete added a comment.

Rebase and friendly ping :)


Repository:
  rC Clang

https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/FixIt/fixit-cxx0x.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp

Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,8 +53,11 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
   }
 
   // We support init-captures in C++11 as an extension.
Index: test/FixIt/fixit-cxx0x.cpp
===
--- test/FixIt/fixit-cxx0x.cpp
+++ test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // expected-error{{lambda requires '()' before return type}}
+
+  delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+  delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
 }
 
 #define bar "bar"
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2976,8 +2976,38 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
+
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+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::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getBeginLoc();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RightBracketLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getEndLoc(), 0,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+}
+
 ArrayDelete = true;
 BalancedDelimiterTracker T(*this, tok::l_square);
 
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -102,6 +102,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 159832.
Rakete added a comment.

Rebase + friendly ping :)


Repository:
  rC Clang

https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/FixIt/fixit-cxx0x.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp

Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,8 +53,11 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
   }
 
   // We support init-captures in C++11 as an extension.
Index: test/FixIt/fixit-cxx0x.cpp
===
--- test/FixIt/fixit-cxx0x.cpp
+++ test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // expected-error{{lambda requires '()' before return type}}
+
+  delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+  delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
 }
 
 #define bar "bar"
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2946,8 +2946,38 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
+
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+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::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getLocStart();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RightBracketLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getLocEnd(), 0,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+}
+
 ArrayDelete = true;
 BalancedDelimiterTracker T(*this, tok::l_square);
 
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 157642.
Rakete added a comment.

Addressed review comments.

Note that clang doesn't support the fourth kind of lambda yet ([]<>), because 
https://reviews.llvm.org/D36527 hasn't been merged yet, so I didn't add a test 
case for that one.


Repository:
  rC Clang

https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/FixIt/fixit-cxx0x.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp

Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,8 +53,11 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
   }
 
   // We support init-captures in C++11 as an extension.
Index: test/FixIt/fixit-cxx0x.cpp
===
--- test/FixIt/fixit-cxx0x.cpp
+++ test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // expected-error{{lambda requires '()' before return type}}
+
+  delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+  delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
 }
 
 #define bar "bar"
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2946,8 +2946,38 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
+
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+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::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getLocStart();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RightBracketLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getLocEnd(), 0,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+}
+
 ArrayDelete = true;
 BalancedDelimiterTracker T(*this, tok::l_square);
 
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : 

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 157611.
Rakete added a comment.

Rebased + friendly ping


Repository:
  rC Clang

https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete 
interpreted as 'delete[]'}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,7 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete 
interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2946,15 +2946,44 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+if (Next.isOneOf(tok::l_brace, tok::less) ||
+(Next.is(tok::l_paren) &&
+ GetLookAheadToken(3).isOneOf(tok::r_paren, tok::identifier) &&
+ GetLookAheadToken(4).is(tok::identifier))) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getLocStart();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RightBracketLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getLocEnd(), 1,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+} else {
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, 
InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 155336.
Rakete edited the summary of this revision.
Rakete added a comment.

Addressed review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete 
interpreted as 'delete[]'}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,7 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete 
interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2942,15 +2942,44 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+if (Next.isOneOf(tok::l_brace, tok::less) ||
+(Next.is(tok::l_paren) &&
+ GetLookAheadToken(3).isOneOf(tok::r_paren, tok::identifier) &&
+ GetLookAheadToken(4).is(tok::identifier))) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getLocStart();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RightBracketLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getLocEnd(), 1,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+} else {
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, 
InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp

[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.

Thanks!




Comment at: lib/Parse/ParseExprCXX.cpp:2956
+const Token Next = GetLookAheadToken(2);
+const auto GetAfter = [this] { return GetLookAheadToken(3); };
+

I don't think this lambda is useful: since you're not caching the result of 
`GetLookAheadToken(3)` between calls, you may as well call it directly below.



Comment at: lib/Parse/ParseExprCXX.cpp:2961-2962
+(Next.is(tok::l_paren) &&
+ (GetAfter().is(tok::r_paren) ||
+  (GetAfter().is(tok::identifier) &&
+   GetLookAheadToken(4).is(tok::identifier) {

Use `GetLookAheadToken(3).isOneOf(tok::r_paren, tok::identifier)` here.



Comment at: lib/Parse/ParseExprCXX.cpp:2971-2980
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, StartLoc.getLocWithOffset(1));
+
+  Diag(StartLoc, diag::note_lambda_after_delete)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getLocEnd(), 1,

Because we can be very confident the fix is correct, and we're recovering as if 
the fix were applied, you can combine the `err_` diagnostic and the `note_` 
diagnostic into one. That way, tools like `clang -fixit` will apply your fixit.



Comment at: lib/Parse/ParseExprCXX.cpp:2972
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, StartLoc.getLocWithOffset(1));
+

If you want the location of the `]` token for the end of the range here, it'd 
be best to save `NextToken().getLocation()` prior to calling 
`ParseLambdaExpression()`. Adding 1 to the location of the `[` token isn't 
necessarily the right location (for instance, if there's whitespace between the 
`[` and `]`, or if the left square bracket is spelled as `<:`).


https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 154882.
Rakete added a comment.

Rebased + friendly ping :)


https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp

Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,7 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+  // expected-note@-1 {{add parentheses around the lambda}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,8 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+// expected-note@-1 {{add parentheses around the lambda}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2951,15 +2951,47 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+const Token Next = GetLookAheadToken(2);
+const auto GetAfter = [this] { return GetLookAheadToken(3); };
+
+// Basic lookahead to check if we have a lambda expression.
+if (Next.isOneOf(tok::l_brace, tok::less) ||
+(Next.is(tok::l_paren) &&
+ (GetAfter().is(tok::r_paren) ||
+  (GetAfter().is(tok::identifier) &&
+   GetLookAheadToken(4).is(tok::identifier) {
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getLocStart();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, StartLoc.getLocWithOffset(1));
+
+  Diag(StartLoc, diag::note_lambda_after_delete)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getLocEnd(), 1,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+} else {
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,10 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
+def note_lambda_after_delete : Note<
+  "add parentheses around the lambda">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 153271.
Rakete added a comment.

Addressed review comments :)


https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp

Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,7 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+  // expected-note@-1 {{add parentheses around the lambda}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,8 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+// expected-note@-1 {{add parentheses around the lambda}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2951,15 +2951,47 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+const Token Next = GetLookAheadToken(2);
+const auto GetAfter = [this] { return GetLookAheadToken(3); };
+
+// Basic lookahead to check if we have a lambda expression.
+if (Next.isOneOf(tok::l_brace, tok::less) ||
+(Next.is(tok::l_paren) &&
+ (GetAfter().is(tok::r_paren) ||
+  (GetAfter().is(tok::identifier) &&
+   GetLookAheadToken(4).is(tok::identifier) {
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getLocStart();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, StartLoc.getLocWithOffset(1));
+
+  Diag(StartLoc, diag::note_lambda_after_delete)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getLocEnd(), 1,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+} else {
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,10 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
+def note_lambda_after_delete : Note<
+  "add parentheses around the lambda">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added a comment.

@rsmith Should I only check for an identifier, like only "int" and nothing 
else? Because parsing a full type specifier might be a bit expensive, no?


https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added a comment.

@rsmith Which type specifiers should I test for? just `T`? Or also `T*`, `T&`, 
...? Now I'm just checking for an identifier, but anything else would make the 
lookahead more complicated I think. Any ideas?


https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 138335.
Rakete added a comment.

> Did you forget to upload the updated patch? This looks unchanged compared to 
> the prior version.

Whoops, that's true. Sorry...


https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp

Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,7 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+  // expected-note@-1 {{add parentheses around the lambda}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,8 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+// expected-note@-1 {{add parentheses around the lambda}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2903,15 +2903,47 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+const Token Next = GetLookAheadToken(2);
+const Token After = GetLookAheadToken(3);
+const Token Last = GetLookAheadToken(4);
+
+// Basic lookahead to check if we have a lambda expression.
+if (Next.isOneOf(tok::l_brace, tok::less) ||
+(Next.is(tok::l_paren) && (
+  After.is(tok::r_paren) || (After.is(tok::identifier) &&
+Last.is(tok::identifier))
+))) {
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = TryParseLambdaExpression();
+  if (!Lambda.isInvalid()) {
+SourceLocation StartLoc = Lambda.get()->getLocStart();
+Diag(Start, diag::err_lambda_after_delete)
+<< SourceRange(Start, StartLoc.getLocWithOffset(1));
+
+SourceLocation BeforeBracket = StartLoc.getLocWithOffset(-1);
+Diag(BeforeBracket, diag::note_lambda_after_delete)
+<< FixItHint::CreateInsertion(BeforeBracket, "(")
+<< FixItHint::CreateInsertion(
+   Lambda.get()->getLocEnd().getLocWithOffset(1), ")");
+
+// Evaluate any postfix expressions used on the lambda.
+Lambda = ParsePostfixExpressionSuffix(Lambda);
+return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+  Lambda.get());
+  }
+  else
+return ExprError();
+} else {
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,10 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
+def note_lambda_after_delete : Note<
+  "add parentheses around the lambda">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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.

Did you forget to upload the updated patch? This looks unchanged compared to 
the prior version.


https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 130020.
Rakete added a comment.

Addressed review comments :)


https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp

Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -31,9 +31,9 @@
   delete [&]{ return (int*)0; }();
 }
 
-void bad_deletes()
-{
-  // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
-}
+void bad_deletes()
+{
+  // 'delete []' is always array delete, per [expr.delete]p1.
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+  // expected-note@-1 {{add parentheses around the lambda}}
+}
\ No newline at end of file
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,8 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+// expected-note@-1 {{add parentheses around the lambda}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2901,15 +2901,45 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
+TentativeParsingAction TPA(*this);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+// Basic lookahead to check if we have a lambda expression. If we
+// encounter two braces with a semicolon, we can be pretty sure
+// that this is a lambda, not say a compound literal. 
+if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+(NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+!SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+  TPA.Revert();
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+} else {
+  TPA.Revert();
+
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = TryParseLambdaExpression();
+  if (!Lambda.isInvalid()) {
+SourceLocation StartLoc = Lambda.get()->getLocStart();
+Diag(Start, diag::err_lambda_after_delete)
+<< SourceRange(Start, StartLoc.getLocWithOffset(1));
+
+SourceLocation BeforeBracket = StartLoc.getLocWithOffset(-1);
+Diag(BeforeBracket, diag::note_lambda_after_delete)
+<< FixItHint::CreateInsertion(BeforeBracket, "(")
+<< FixItHint::CreateInsertion(
+   Lambda.get()->getLocEnd().getLocWithOffset(1), ")");
+
+// Evaluate any postfix expressions used on the lambda.
+Lambda = ParsePostfixExpressionSuffix(Lambda);
+return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+  Lambda.get());
+  }
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -96,12 +96,16 @@
   InGroup, DefaultIgnore;
 def warn_cxx98_compat_alignof : Warning<
   "alignof expressions are incompatible with C++98">,
-  InGroup, DefaultIgnore;
-def ext_alignof_expr : ExtWarn<
-  "%0 applied to an expression is a GNU extension">, InGroup;
-
-def warn_microsoft_dependent_exists : Warning<
-  "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 
+  InGroup, DefaultIgnore;
+def ext_alignof_expr : ExtWarn<
+  "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
+def 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added inline comments.



Comment at: lib/Parse/ParseExprCXX.cpp:2906-2912
+// Basic lookahead to check if we have a lambda expression. If we
+// encounter two braces with a semicolon, we can be pretty sure
+// that this is a lambda, not say a compound literal. 
+if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+(NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+!SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+  TPA.Revert();

rsmith wrote:
> This seems error-prone. Given:
> 
> ```
> for (item *p = first, *oldp = p; p; p = p->next, delete [] oldp, oldp = p) {
>   /*...*/;
> }
> ```
> 
> ... we'll decide the //delete-expression// is followed by a lambda. Likewise 
> for cases like:
> 
> ```
> delete [] f->getPtr([] {
>   return blah;
> });
> ```
> 
> Our goal here should be a fast heuristic (don't use lookahead except when 
> you're already confident you have a lambda) and zero false positives. How 
> about these heuristics instead:
> 
> Assume that the `delete []` is actually `delete` followed by a lambda if 
> either:
> 
>  * The next token is `{` or `<`, or
>  * The next token is `(` and either
> * the following token is `)` or
> * the following tokens are a type specifier followed by an identifier
> 
> This should have no false positives, and only has false negatives if a lambda 
> has an unnamed parameter or a parameter with a non-trivial parameter type. 
> (For the last condition, we could try to tentatively parse an entire 
> parameter and see if it has a name, which would handle all cases except an 
> expression/declaration ambiguity in the parameter declaration, but that seems 
> like overkill to me. This is already performing more lookahead than I'd like, 
> but `delete []` expressions are rare enough that using two lookahead tokens 
> for an improved error message seems OK.)
Ah, I see. I had no idea that lookaheads were that expensive. Thanks for 
finding a better algorithm than me :)



https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:2906-2912
+// Basic lookahead to check if we have a lambda expression. If we
+// encounter two braces with a semicolon, we can be pretty sure
+// that this is a lambda, not say a compound literal. 
+if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+(NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+!SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+  TPA.Revert();

This seems error-prone. Given:

```
for (item *p = first, *oldp = p; p; p = p->next, delete [] oldp, oldp = p) {
  /*...*/;
}
```

... we'll decide the //delete-expression// is followed by a lambda. Likewise 
for cases like:

```
delete [] f->getPtr([] {
  return blah;
});
```

Our goal here should be a fast heuristic (don't use lookahead except when 
you're already confident you have a lambda) and zero false positives. How about 
these heuristics instead:

Assume that the `delete []` is actually `delete` followed by a lambda if either:

 * The next token is `{` or `<`, or
 * The next token is `(` and either
* the following token is `)` or
* the following tokens are a type specifier followed by an identifier

This should have no false positives, and only has false negatives if a lambda 
has an unnamed parameter or a parameter with a non-trivial parameter type. (For 
the last condition, we could try to tentatively parse an entire parameter and 
see if it has a name, which would handle all cases except an 
expression/declaration ambiguity in the parameter declaration, but that seems 
like overkill to me. This is already performing more lookahead than I'd like, 
but `delete []` expressions are rare enough that using two lookahead tokens for 
an improved error message seems OK.)


https://reviews.llvm.org/D36357



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 128668.
Rakete added a comment.

Rebased + friendly 2018 ping :)


https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete 
interpreted as 'delete[]'}}
+  // expected-note@-1 {{add parentheses around the lambda}}
 }
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,8 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete 
interpreted as 'delete[]'}}
+// expected-note@-1 {{add parentheses around the lambda}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2901,15 +2901,45 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
+TentativeParsingAction TPA(*this);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+// Basic lookahead to check if we have a lambda expression. If we
+// encounter two braces with a semicolon, we can be pretty sure
+// that this is a lambda, not say a compound literal. 
+if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+(NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+!SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+  TPA.Revert();
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+} else {
+  TPA.Revert();
+
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = TryParseLambdaExpression();
+  if (!Lambda.isInvalid()) {
+SourceLocation StartLoc = Lambda.get()->getLocStart();
+Diag(Start, diag::err_lambda_after_delete)
+<< SourceRange(Start, StartLoc.getLocWithOffset(1));
+
+SourceLocation BeforeBracket = StartLoc.getLocWithOffset(-1);
+Diag(BeforeBracket, diag::note_lambda_after_delete)
+<< FixItHint::CreateInsertion(BeforeBracket, "(")
+<< FixItHint::CreateInsertion(
+   Lambda.get()->getLocEnd().getLocWithOffset(1), ")");
+
+// Evaluate any postfix expressions used on the lambda.
+Lambda = ParsePostfixExpressionSuffix(Lambda);
+return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+  Lambda.get());
+  }
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,10 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, 
InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
+def note_lambda_after_delete : Note<
+  "add parentheses around the lambda">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return 

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 125189.
Rakete added a comment.

Updated error message, added a FixItHint + a rebase and friendly ping :)


https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp

Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,7 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+  // expected-note@-1 {{add parentheses around the lambda}}
 }
+
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,8 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
+// expected-note@-1 {{add parentheses around the lambda}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2896,15 +2896,45 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
+TentativeParsingAction TPA(*this);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+// Basic lookahead to check if we have a lambda expression. If we
+// encounter two braces with a semicolon, we can be pretty sure
+// that this is a lambda, not say a compound literal. 
+if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+(NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+!SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+  TPA.Revert();
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+} else {
+  TPA.Revert();
+
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = TryParseLambdaExpression();
+  if (!Lambda.isInvalid()) {
+SourceLocation StartLoc = Lambda.get()->getLocStart();
+Diag(Start, diag::err_lambda_after_delete)
+<< SourceRange(Start, StartLoc.getLocWithOffset(1));
+
+SourceLocation BeforeBracket = StartLoc.getLocWithOffset(-1);
+Diag(BeforeBracket, diag::note_lambda_after_delete)
+<< FixItHint::CreateInsertion(BeforeBracket, "(")
+<< FixItHint::CreateInsertion(
+   Lambda.get()->getLocEnd().getLocWithOffset(1), ")");
+
+// Evaluate any postfix expressions used on the lambda.
+Lambda = ParsePostfixExpressionSuffix(Lambda);
+return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+  Lambda.get());
+  }
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,10 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, InGroup;
+def err_lambda_after_delete : Error<
+  "'[]' after delete interpreted as 'delete[]'">;
+def note_lambda_after_delete : Note<
+  "add parentheses around the lambda">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 115881.
Rakete added a project: clang.
Rakete added a comment.

Used better diagnostic id.


https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,5 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error{{missing parentheses 
around lambda expression}}
 }
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,7 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error{{missing parentheses 
around lambda expression}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2890,15 +2890,39 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
+TentativeParsingAction TPA(*this);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+// Basic lookahead to check if we have a lambda expression. If we
+// encounter two braces with a semicolon, we can be pretty sure
+// that this is a lambda, not say a compound literal. 
+if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+(NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+!SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+  TPA.Revert();
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+} else {
+  TPA.Revert();
+
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = TryParseLambdaExpression();
+  if (!Lambda.isInvalid()) {
+SourceLocation StartLoc = Lambda.get()->getLocStart();
+Diag(StartLoc, diag::err_lambda_after_delete)
+  << SourceRange(StartLoc, Lambda.get()->getLocEnd());
+
+// Evaluate any postfix expressions used on the lambda.
+Lambda = ParsePostfixExpressionSuffix(Lambda);
+return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+  Lambda.get());
+  }
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, 
InGroup;
+def err_lambda_after_delete : Error<
+  "missing parentheses around lambda expression">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,5 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error{{missing parentheses around lambda expression}}
 }
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,7 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, 

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

[Blitz Rakete via Phabricator via cfe-commits]

Rakete updated this revision to Diff 112103.
Rakete added a comment.

Rebased and friendly ping.


https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,5 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error{{missing parentheses 
around lambda expression}}
 }
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,7 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error{{missing parentheses 
around lambda expression}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2885,15 +2885,39 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
+TentativeParsingAction TPA(*this);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+// Basic lookahead to check if we have a lambda expression. If we
+// encounter two braces with a semicolon, we can be pretty sure
+// that this is a lambda, not say a compound literal. 
+if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+(NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+!SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+  TPA.Revert();
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+} else {
+  TPA.Revert();
+
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = TryParseLambdaExpression();
+  if (!Lambda.isInvalid()) {
+SourceLocation StartLoc = Lambda.get()->getLocStart();
+Diag(StartLoc, diag::err_lambda_after_array_delete)
+  << SourceRange(StartLoc, Lambda.get()->getLocEnd());
+
+// Evaluate any postfix expressions used on the lambda.
+Lambda = ParsePostfixExpressionSuffix(Lambda);
+return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+  Lambda.get());
+  }
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, 
InGroup;
+def err_lambda_after_array_delete : Error<
+  "missing parentheses around lambda expression">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,5 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error{{missing parentheses around lambda expression}}
 }
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,7 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] 

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

[Blitz Rakete via Phabricator via cfe-commits]

Rakete created this revision.

This adds a new error for missing parentheses around lambdas in delete 
operators.

  int main() {
delete []() { return new int(); }();
  }

This will result in:

  test.cpp:2:10: error: missing parentheses around lambda expression
delete []() { return new int(); }();
   ^~~
  1 error generated.




https://reviews.llvm.org/D36357

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseExprCXX.cpp
  test/Parser/cxx0x-lambda-expressions.cpp
  test/SemaCXX/new-delete-0x.cpp


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,5 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error{{missing parentheses 
around lambda expression}}
 }
Index: test/Parser/cxx0x-lambda-expressions.cpp
===
--- test/Parser/cxx0x-lambda-expressions.cpp
+++ test/Parser/cxx0x-lambda-expressions.cpp
@@ -53,7 +53,7 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error{{missing parentheses 
around lambda expression}}
 delete [&] { return new int; } (); // ok, lambda
   }
 
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -2884,15 +2884,39 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
-ArrayDelete = true;
-BalancedDelimiterTracker T(*this, tok::l_square);
+TentativeParsingAction TPA(*this);
 
-T.consumeOpen();
-T.consumeClose();
-if (T.getCloseLocation().isInvalid())
-  return ExprError();
+// Basic lookahead to check if we have a lambda expression. If we
+// encounter two braces with a semicolon, we can be pretty sure
+// that this is a lambda, not say a compound literal. 
+if (!SkipUntil(tok::l_brace, SkipUntilFlags::StopAtSemi) ||
+(NextToken().isNot(tok::r_brace) && !SkipUntil(tok::semi)) ||
+!SkipUntil(tok::r_brace, SkipUntilFlags::StopAtSemi)) {
+  TPA.Revert();
+  ArrayDelete = true;
+  BalancedDelimiterTracker T(*this, tok::l_square);
+
+  T.consumeOpen();
+  T.consumeClose();
+  if (T.getCloseLocation().isInvalid())
+return ExprError();
+} else {
+  TPA.Revert();
+
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis
+  // to disambiguate it from 'delete[]'. 
+  ExprResult Lambda = TryParseLambdaExpression();
+  if (!Lambda.isInvalid()) {
+SourceLocation StartLoc = Lambda.get()->getLocStart();
+Diag(StartLoc, diag::err_lambda_after_array_delete)
+  << SourceRange(StartLoc, Lambda.get()->getLocEnd());
+
+// Evaluate any postfix expressions used on the lambda.
+Lambda = ParsePostfixExpressionSuffix(Lambda);
+return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+  Lambda.get());
+  }
+}
   }
 
   ExprResult Operand(ParseCastExpression(false));
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -99,6 +99,8 @@
   InGroup, DefaultIgnore;
 def ext_alignof_expr : ExtWarn<
   "%0 applied to an expression is a GNU extension">, 
InGroup;
+def err_lambda_after_array_delete : Error<
+  "missing parentheses around lambda expression">;
 
 def warn_microsoft_dependent_exists : Warning<
   "dependent %select{__if_not_exists|__if_exists}0 declarations are ignored">, 


Index: test/SemaCXX/new-delete-0x.cpp
===
--- test/SemaCXX/new-delete-0x.cpp
+++ test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,5 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error{{missing parentheses around lambda expression}}
 }
Index: test/Parser/cxx0x-lambda-expressions.cpp