[PATCH] D64632: [clang-format] Don't detect call to ObjC class method as C++11 attribute specifier

[Robbie Gibson via Phabricator via cfe-commits]

rkgibson2 created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64632

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7027,6 +7027,10 @@
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake a call to the Objective-C method named
+  // "class" inside an array literal as attributes.
+  verifyFormat("@[ [NSArray class] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -400,9 +400,11 @@
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]]. 'class' is a common ObjC method selector, so allow it as
+  // well.
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||
+  AttrTok->startsSequence(tok::identifier, tok::kw_class) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))
 return false;
   if (AttrTok->is(tok::ellipsis))


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7027,6 +7027,10 @@
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake a call to the Objective-C method named
+  // "class" inside an array literal as attributes.
+  verifyFormat("@[ [NSArray class] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -400,9 +400,11 @@
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]]. 'class' is a common ObjC method selector, so allow it as
+  // well.
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||
+  AttrTok->startsSequence(tok::identifier, tok::kw_class) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))
 return false;
   if (AttrTok->is(tok::ellipsis))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64632: [clang-format] Don't detect call to ObjC class method as C++11 attribute specifier

[Robbie Gibson via Phabricator via cfe-commits]

rkgibson2 marked an inline comment as done.
rkgibson2 added a comment.

In D64632#1582793 , @benhamilton wrote:

> Thanks for the fix. One question: how does the real Clang parser deal with 
> this case? Is it something that's actually ambiguous in the ObjC++ grammar, I 
> wonder?


I am not sure about real Clang. I actually don't know much about how it works. 
I just ran into this bug and wanted to see if it was fixable. My guess is the @ 
sign marks the bracket as an array literal. In clang-format, 
isCpp11AttributeSpecifier is checked before checking whether the previous token 
is @.




Comment at: clang/lib/Format/TokenAnnotator.cpp:400-413
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]]. 'class' is a common ObjC method selector, so allow it as
+  // well.
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||

benhamilton wrote:
> Maybe we should check the token before AttrTok to see if it's `tok::at`, 
> rather than checking for an identifier followed by `tok::kw_class`?
> 
> I don't think there's any valid C++11 attribute specifier sequence of `@[[]]`.
> 
Ok, sure. I considered that at first but I'm not familiar with C++11 enough to 
say. I thought it wasn't valid, but I wasn't sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64632



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

[PATCH] D64632: [clang-format] Don't detect call to ObjC class method as C++11 attribute specifier

[Robbie Gibson via Phabricator via cfe-commits]

rkgibson2 updated this revision to Diff 209496.
rkgibson2 added a comment.

Respond to comments and change detection method


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64632

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7027,6 +7027,10 @@
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake a call to the Objective-C method named
+  // "class" inside an array literal as attributes.
+  verifyFormat("@[ [NSArray class] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -388,6 +388,10 @@
   bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
 if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
   return false;
+// Objective-C array literal and message send.
+if (Tok.Previous && Tok.Previous->is(tok::at)) {
+  return false;
+}
 const FormatToken *AttrTok = Tok.Next->Next;
 if (!AttrTok)
   return false;
@@ -400,7 +404,7 @@
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]].
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7027,6 +7027,10 @@
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake a call to the Objective-C method named
+  // "class" inside an array literal as attributes.
+  verifyFormat("@[ [NSArray class] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -388,6 +388,10 @@
   bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
 if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
   return false;
+// Objective-C array literal and message send.
+if (Tok.Previous && Tok.Previous->is(tok::at)) {
+  return false;
+}
 const FormatToken *AttrTok = Tok.Next->Next;
 if (!AttrTok)
   return false;
@@ -400,7 +404,7 @@
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]].
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64632: [clang-format] Don't detect call to ObjC class method as C++11 attribute specifier

[Robbie Gibson via Phabricator via cfe-commits]

rkgibson2 updated this revision to Diff 209504.
rkgibson2 added a comment.

Update comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64632

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7027,6 +7027,10 @@
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake a call to the Objective-C method named
+  // "class" inside an array literal as attributes.
+  verifyFormat("@[ [NSArray class] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -388,6 +388,10 @@
   bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
 if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
   return false;
+// The first square bracket is part of an ObjC array literal
+if (Tok.Previous && Tok.Previous->is(tok::at)) {
+  return false;
+}
 const FormatToken *AttrTok = Tok.Next->Next;
 if (!AttrTok)
   return false;
@@ -400,7 +404,7 @@
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]].
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7027,6 +7027,10 @@
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake a call to the Objective-C method named
+  // "class" inside an array literal as attributes.
+  verifyFormat("@[ [NSArray class] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -388,6 +388,10 @@
   bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
 if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
   return false;
+// The first square bracket is part of an ObjC array literal
+if (Tok.Previous && Tok.Previous->is(tok::at)) {
+  return false;
+}
 const FormatToken *AttrTok = Tok.Next->Next;
 if (!AttrTok)
   return false;
@@ -400,7 +404,7 @@
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]].
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64632: [clang-format] Don't detect call to ObjC class method as C++11 attribute specifier

[Robbie Gibson via Phabricator via cfe-commits]

rkgibson2 updated this revision to Diff 209777.
rkgibson2 added a comment.

Add more tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64632

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7027,6 +7027,12 @@
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake Objective-C method inside array literals
+  // as attributes, even if those method names are also keywords.
+  verifyFormat("@[ [foo bar] ];");
+  verifyFormat("@[ [NSArray class] ];");
+  verifyFormat("@[ [foo enum] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -388,6 +388,10 @@
   bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
 if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
   return false;
+// The first square bracket is part of an ObjC array literal
+if (Tok.Previous && Tok.Previous->is(tok::at)) {
+  return false;
+}
 const FormatToken *AttrTok = Tok.Next->Next;
 if (!AttrTok)
   return false;
@@ -400,7 +404,7 @@
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]].
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7027,6 +7027,12 @@
   // On the other hand, we still need to correctly find array subscripts.
   verifyFormat("int a = std::vector{1, 2, 3}[0];");
 
+  // Make sure that we do not mistake Objective-C method inside array literals
+  // as attributes, even if those method names are also keywords.
+  verifyFormat("@[ [foo bar] ];");
+  verifyFormat("@[ [NSArray class] ];");
+  verifyFormat("@[ [foo enum] ];");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -388,6 +388,10 @@
   bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
 if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
   return false;
+// The first square bracket is part of an ObjC array literal
+if (Tok.Previous && Tok.Previous->is(tok::at)) {
+  return false;
+}
 const FormatToken *AttrTok = Tok.Next->Next;
 if (!AttrTok)
   return false;
@@ -400,7 +404,7 @@
 while (AttrTok && !AttrTok->startsSequence(tok::r_square, tok::r_square)) {
   // ObjC message send. We assume nobody will use : in a C++11 attribute
   // specifier parameter, although this is technically valid:
-  // [[foo(:)]]
+  // [[foo(:)]].
   if (AttrTok->is(tok::colon) ||
   AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64632: [clang-format] Don't detect call to ObjC class method as C++11 attribute specifier

[Robbie Gibson via Phabricator via cfe-commits]

rkgibson2 added a comment.

I don't think I have commit access. Could someone land this for me?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64632



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