[PATCH] D26916: [ObjC] Avoid a @try/@finally/@autoreleasepool fixit when parsing an expression
This revision was automatically updated to reflect the committed changes.
Closed by commit rL288334: [ObjC] Avoid a @try/@finally/@autoreleasepool fixit
when parsing an expression (authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D26916?vs=78728=79893#toc
Repository:
rL LLVM
https://reviews.llvm.org/D26916
Files:
cfe/trunk/lib/Parse/ParseObjc.cpp
cfe/trunk/test/Parser/objc-at-directive-fixit.m
Index: cfe/trunk/test/Parser/objc-at-directive-fixit.m
===
--- cfe/trunk/test/Parser/objc-at-directive-fixit.m
+++ cfe/trunk/test/Parser/objc-at-directive-fixit.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0 -verify
-fobjc-exceptions %s
+// RUN: not %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0
-fdiagnostics-parseable-fixits -fobjc-exceptions %s 2>&1 | FileCheck %s
+
+// rdar://19669565
+
+void bar(int x);
+
+void f() {
+ @try { }
+ @finally { }
+ @autoreleasepool { }
+
+ // Provide a fixit when we are parsing a standalone statement
+ @tr { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"try"
+ @finaly { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:10}:"finally"
+ @autorelpool { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:15}:"autoreleasepool"
+
+ // Ensure that no fixit is given when parsing expressions
+ // CHECK-NOT: fix-it
+ id thing = @autoreleasepool { }; // expected-error {{unexpected '@' in
program}}
+ (void)@tr { }; // expected-error {{unexpected '@' in program}}
+ bar(@final { }); // expected-error {{unexpected '@' in program}}
+ for(@auto;;) { } // expected-error {{unexpected '@' in program}}
+ [@try]; // expected-error {{unexpected '@' in program}}
+}
Index: cfe/trunk/lib/Parse/ParseObjc.cpp
===
--- cfe/trunk/lib/Parse/ParseObjc.cpp
+++ cfe/trunk/lib/Parse/ParseObjc.cpp
@@ -2773,6 +2773,7 @@
return Actions.ActOnNullStmt(Tok.getLocation());
}
+ ExprStatementTokLoc = AtLoc;
ExprResult Res(ParseExpressionWithLeadingAt(AtLoc));
if (Res.isInvalid()) {
// If the expression is invalid, skip ahead to the next semicolon. Not
@@ -2869,7 +2870,11 @@
return ParseAvailabilityCheckExpr(AtLoc);
default: {
const char *str = nullptr;
-if (GetLookAheadToken(1).is(tok::l_brace)) {
+// Only provide the @try/@finally/@autoreleasepool fixit when we're
sure
+// that this is a proper statement where such directives could actually
+// occur.
+if (GetLookAheadToken(1).is(tok::l_brace) &&
+ExprStatementTokLoc == AtLoc) {
char ch = Tok.getIdentifierInfo()->getNameStart()[0];
str =
ch == 't' ? "try"
Index: cfe/trunk/test/Parser/objc-at-directive-fixit.m
===
--- cfe/trunk/test/Parser/objc-at-directive-fixit.m
+++ cfe/trunk/test/Parser/objc-at-directive-fixit.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0 -verify -fobjc-exceptions %s
+// RUN: not %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0 -fdiagnostics-parseable-fixits -fobjc-exceptions %s 2>&1 | FileCheck %s
+
+// rdar://19669565
+
+void bar(int x);
+
+void f() {
+ @try { }
+ @finally { }
+ @autoreleasepool { }
+
+ // Provide a fixit when we are parsing a standalone statement
+ @tr { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"try"
+ @finaly { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:10}:"finally"
+ @autorelpool { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:15}:"autoreleasepool"
+
+ // Ensure that no fixit is given when parsing expressions
+ // CHECK-NOT: fix-it
+ id thing = @autoreleasepool { }; // expected-error {{unexpected '@' in program}}
+ (void)@tr { }; // expected-error {{unexpected '@' in program}}
+ bar(@final { }); // expected-error {{unexpected '@' in program}}
+ for(@auto;;) { } // expected-error {{unexpected '@' in program}}
+ [@try]; // expected-error {{unexpected '@' in program}}
+}
Index: cfe/trunk/lib/Parse/ParseObjc.cpp
===
--- cfe/trunk/lib/Parse/ParseObjc.cpp
+++ cfe/trunk/lib/Parse/ParseObjc.cpp
@@ -2773,6 +2773,7 @@
return Actions.ActOnNullStmt(Tok.getLocation());
}
+ ExprStatementTokLoc = AtLoc;
ExprResult Res(ParseExpressionWithLeadingAt(AtLoc));
if (Res.isInvalid()) {
// If the expression is invalid, skip ahead to the next semicolon. Not
@@ -2869,7 +2870,11 @@
return
[PATCH] D26916: [ObjC] Avoid a @try/@finally/@autoreleasepool fixit when parsing an expression
bruno accepted this revision. bruno added a comment. This revision is now accepted and ready to land. Thanks for the explanation, LGTM Repository: rL LLVM https://reviews.llvm.org/D26916 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26916: [ObjC] Avoid a @try/@finally/@autoreleasepool fixit when parsing an expression
arphaman added inline comments.
Comment at: lib/Parse/ParseObjc.cpp:2877
+if (GetLookAheadToken(1).is(tok::l_brace) &&
+ExprStatementTokLoc == AtLoc) {
char ch = Tok.getIdentifierInfo()->getNameStart()[0];
bruno wrote:
> Does this only triggers when `Res.isInvalid()` returns true in the first part
> of the patch? I wonder if it's also safe to allow `ExprStatementTokLoc =
> AtLoc;` for every path or only when it fails.
Yes, this code will always flow to the body of the if with `Res.isInvalid`, but
only after this code is executed, so we need to set `ExprStatementTokLoc`
before the check for `Res.isInvalid`. As well as that, all users of
`ExprStatementTokLoc` currently care only about the current location where the
current statement is being parsed (they check to see if some expression
location matches it), so it should be set before a method call like
`ParseExpressionWithLeadingAt`, and it doesn't have to be cleared as the next
statement will have a different location anyway. So it seems safe to set
`ExprStatementTokLoc = AtLoc` as it follows the intended convention.
Repository:
rL LLVM
https://reviews.llvm.org/D26916
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26916: [ObjC] Avoid a @try/@finally/@autoreleasepool fixit when parsing an expression
bruno added a comment.
Hi Alex, thanks for working on this
Comment at: lib/Parse/ParseObjc.cpp:2877
+if (GetLookAheadToken(1).is(tok::l_brace) &&
+ExprStatementTokLoc == AtLoc) {
char ch = Tok.getIdentifierInfo()->getNameStart()[0];
Does this only triggers when `Res.isInvalid()` returns true in the first part
of the patch? I wonder if it's also safe to allow `ExprStatementTokLoc =
AtLoc;` for every path or only when it fails.
Repository:
rL LLVM
https://reviews.llvm.org/D26916
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26916: [ObjC] Avoid a @try/@finally/@autoreleasepool fixit when parsing an expression
arphaman created this revision.
arphaman added reviewers: manmanren, bruno.
arphaman added a subscriber: cfe-commits.
arphaman set the repository for this revision to rL LLVM.
This patch ensures that the typo fixit for the `@try/@finally/@autoreleasepool
{ }` directive is shown only when we're parsing an actual statement where such
directives can actually be present.
Repository:
rL LLVM
https://reviews.llvm.org/D26916
Files:
lib/Parse/ParseObjc.cpp
test/Parser/objc-at-directive-fixit.m
Index: test/Parser/objc-at-directive-fixit.m
===
--- /dev/null
+++ test/Parser/objc-at-directive-fixit.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0 -verify
-fobjc-exceptions %s
+// RUN: not %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0
-fdiagnostics-parseable-fixits -fobjc-exceptions %s 2>&1 | FileCheck %s
+
+// rdar://19669565
+
+void bar(int x);
+
+void f() {
+ @try { }
+ @finally { }
+ @autoreleasepool { }
+
+ // Provide a fixit when we are parsing a standalone statement
+ @tr { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"try"
+ @finaly { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:10}:"finally"
+ @autorelpool { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:15}:"autoreleasepool"
+
+ // Ensure that no fixit is given when parsing expressions
+ // CHECK-NOT: fix-it
+ id thing = @autoreleasepool { }; // expected-error {{unexpected '@' in
program}}
+ (void)@tr { }; // expected-error {{unexpected '@' in program}}
+ bar(@final { }); // expected-error {{unexpected '@' in program}}
+ for(@auto;;) { } // expected-error {{unexpected '@' in program}}
+ [@try]; // expected-error {{unexpected '@' in program}}
+}
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -2773,6 +2773,7 @@
return Actions.ActOnNullStmt(Tok.getLocation());
}
+ ExprStatementTokLoc = AtLoc;
ExprResult Res(ParseExpressionWithLeadingAt(AtLoc));
if (Res.isInvalid()) {
// If the expression is invalid, skip ahead to the next semicolon. Not
@@ -2869,7 +2870,11 @@
return ParseAvailabilityCheckExpr(AtLoc);
default: {
const char *str = nullptr;
-if (GetLookAheadToken(1).is(tok::l_brace)) {
+// Only provide the @try/@finally/@autoreleasepool fixit when we're
sure
+// that this is a proper statement where such directives could actually
+// occur.
+if (GetLookAheadToken(1).is(tok::l_brace) &&
+ExprStatementTokLoc == AtLoc) {
char ch = Tok.getIdentifierInfo()->getNameStart()[0];
str =
ch == 't' ? "try"
Index: test/Parser/objc-at-directive-fixit.m
===
--- /dev/null
+++ test/Parser/objc-at-directive-fixit.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0 -verify -fobjc-exceptions %s
+// RUN: not %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.10.0 -fdiagnostics-parseable-fixits -fobjc-exceptions %s 2>&1 | FileCheck %s
+
+// rdar://19669565
+
+void bar(int x);
+
+void f() {
+ @try { }
+ @finally { }
+ @autoreleasepool { }
+
+ // Provide a fixit when we are parsing a standalone statement
+ @tr { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:6}:"try"
+ @finaly { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:10}:"finally"
+ @autorelpool { }; // expected-error {{unexpected '@' in program}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:15}:"autoreleasepool"
+
+ // Ensure that no fixit is given when parsing expressions
+ // CHECK-NOT: fix-it
+ id thing = @autoreleasepool { }; // expected-error {{unexpected '@' in program}}
+ (void)@tr { }; // expected-error {{unexpected '@' in program}}
+ bar(@final { }); // expected-error {{unexpected '@' in program}}
+ for(@auto;;) { } // expected-error {{unexpected '@' in program}}
+ [@try]; // expected-error {{unexpected '@' in program}}
+}
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -2773,6 +2773,7 @@
return Actions.ActOnNullStmt(Tok.getLocation());
}
+ ExprStatementTokLoc = AtLoc;
ExprResult Res(ParseExpressionWithLeadingAt(AtLoc));
if (Res.isInvalid()) {
// If the expression is invalid, skip ahead to the next semicolon. Not
@@ -2869,7 +2870,11 @@
return ParseAvailabilityCheckExpr(AtLoc);
default: {
const char *str = nullptr;
-if
