[PATCH] D46143: [clang-format/ObjC] Use getIdentifierInfo() instead of tok::identifier

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC331067: [clang-format/ObjC] Use getIdentifierInfo() instead 
of tok::identifier (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46143?vs=144357=144377#toc

Repository:
  rC Clang

https://reviews.llvm.org/D46143

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestObjC.cpp


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -25,6 +25,21 @@
 
 namespace {
 
+/// \brief Returns \c true if the token can be used as an identifier in
+/// an Objective-C \c @selector, \c false otherwise.
+///
+/// Because getFormattingLangOpts() always lexes source code as
+/// Objective-C++, C++ keywords like \c new and \c delete are
+/// lexed as tok::kw_*, not tok::identifier, even for Objective-C.
+///
+/// For Objective-C and Objective-C++, both identifiers and keywords
+/// are valid inside @selector(...) (or a macro which
+/// invokes @selector(...)). So, we allow treat any identifier or
+/// keyword as a potential Objective-C selector component.
+static bool canBeObjCSelectorComponent(const FormatToken ) {
+  return Tok.Tok.getIdentifierInfo() != nullptr;
+}
+
 /// \brief A parser that gathers additional information about tokens.
 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
@@ -703,9 +718,10 @@
   Tok->Type = TT_CtorInitializerColon;
 else
   Tok->Type = TT_InheritanceColon;
-  } else if (Tok->Previous->is(tok::identifier) && Tok->Next &&
+  } else if (canBeObjCSelectorComponent(*Tok->Previous) && Tok->Next &&
  (Tok->Next->isOneOf(tok::r_paren, tok::comma) ||
-  Tok->Next->startsSequence(tok::identifier, tok::colon))) {
+  (canBeObjCSelectorComponent(*Tok->Next) && Tok->Next->Next &&
+   Tok->Next->Next->is(tok::colon {
 // This handles a special macro in ObjC code where selectors including
 // the colon are passed as macro arguments.
 Tok->Type = TT_ObjCMethodExpr;
@@ -1346,7 +1362,7 @@
  TT_LeadingJavaAnnotation)) {
 Current.Type = Current.Previous->Type;
   }
-} else if (Current.isOneOf(tok::identifier, tok::kw_new) &&
+} else if (canBeObjCSelectorComponent(Current) &&
// FIXME(bug 36976): ObjC return types shouldn't use 
TT_CastRParen.
Current.Previous && Current.Previous->is(TT_CastRParen) &&
Current.Previous->MatchingParen &&
@@ -2650,7 +2666,7 @@
   if (Line.Type == LT_ObjCMethodDecl) {
 if (Left.is(TT_ObjCMethodSpecifier))
   return true;
-if (Left.is(tok::r_paren) && Right.isOneOf(tok::identifier, tok::kw_new))
+if (Left.is(tok::r_paren) && canBeObjCSelectorComponent(Right))
   // Don't space between ')' and  or ')' and 'new'. 'new' is not a
   // keyword in Objective-C, and '+ (instancetype)new;' is a standard class
   // method declaration.
@@ -3128,6 +3144,7 @@
 for (unsigned i = 0, e = Tok->FakeLParens.size(); i != e; ++i)
   llvm::errs() << Tok->FakeLParens[i] << "/";
 llvm::errs() << " FakeRParens=" << Tok->FakeRParens;
+llvm::errs() << " II=" << Tok->Tok.getIdentifierInfo();
 llvm::errs() << " Text='" << Tok->TokenText << "'\n";
 if (!Tok->Next)
   assert(Tok == Line.Last);
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -945,15 +945,26 @@
" }]) {\n}");
 }
 
-TEST_F(FormatTestObjC, ObjCNew) {
+TEST_F(FormatTestObjC, ObjCCxxKeywords) {
   verifyFormat("+ (instancetype)new {\n"
"  return nil;\n"
"}\n");
   verifyFormat("+ (instancetype)myNew {\n"
"  return [self new];\n"
"}\n");
   verifyFormat("SEL NewSelector(void) { return @selector(new); }\n");
   verifyFormat("SEL MacroSelector(void) { return MACRO(new); }\n");
+  verifyFormat("+ (instancetype)delete {\n"
+   "  return nil;\n"
+   "}\n");
+  verifyFormat("+ (instancetype)myDelete {\n"
+   "  return [self delete];\n"
+   "}\n");
+  verifyFormat("SEL DeleteSelector(void) { return @selector(delete); }\n");
+  verifyFormat("SEL MacroSelector(void) { return MACRO(delete); }\n");
+  verifyFormat("MACRO(new:)\n");
+  verifyFormat("MACRO(delete:)\n");
+  verifyFormat("foo = @{MACRO(new:) : MACRO(delete:)}\n");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -25,6 +25,21 @@
 
 namespace {
 
+/// \brief Returns \c true 

[PATCH] D46143: [clang-format/ObjC] Use getIdentifierInfo() instead of tok::identifier

[Daniel Jasper via Phabricator via cfe-commits]

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

Looks good, thank you.


Repository:
  rC Clang

https://reviews.llvm.org/D46143



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

[PATCH] D46143: [clang-format/ObjC] Use getIdentifierInfo() instead of tok::identifier

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 144357.
benhamilton added a comment.

Add helper canBeObjCSelectorComponent() with comments.


Repository:
  rC Clang

https://reviews.llvm.org/D46143

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -945,15 +945,26 @@
" }]) {\n}");
 }
 
-TEST_F(FormatTestObjC, ObjCNew) {
+TEST_F(FormatTestObjC, ObjCCxxKeywords) {
   verifyFormat("+ (instancetype)new {\n"
"  return nil;\n"
"}\n");
   verifyFormat("+ (instancetype)myNew {\n"
"  return [self new];\n"
"}\n");
   verifyFormat("SEL NewSelector(void) { return @selector(new); }\n");
   verifyFormat("SEL MacroSelector(void) { return MACRO(new); }\n");
+  verifyFormat("+ (instancetype)delete {\n"
+   "  return nil;\n"
+   "}\n");
+  verifyFormat("+ (instancetype)myDelete {\n"
+   "  return [self delete];\n"
+   "}\n");
+  verifyFormat("SEL DeleteSelector(void) { return @selector(delete); }\n");
+  verifyFormat("SEL MacroSelector(void) { return MACRO(delete); }\n");
+  verifyFormat("MACRO(new:)\n");
+  verifyFormat("MACRO(delete:)\n");
+  verifyFormat("foo = @{MACRO(new:) : MACRO(delete:)}\n");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -25,6 +25,21 @@
 
 namespace {
 
+/// \brief Returns \c true if the token can be used as an identifier in
+/// an Objective-C \c @selector, \c false otherwise.
+///
+/// Because getFormattingLangOpts() always lexes source code as
+/// Objective-C++, C++ keywords like \c new and \c delete are
+/// lexed as tok::kw_*, not tok::identifier, even for Objective-C.
+///
+/// For Objective-C and Objective-C++, both identifiers and keywords
+/// are valid inside @selector(...) (or a macro which
+/// invokes @selector(...)). So, we allow treat any identifier or
+/// keyword as a potential Objective-C selector component.
+static bool canBeObjCSelectorComponent(const FormatToken ) {
+  return Tok.Tok.getIdentifierInfo() != nullptr;
+}
+
 /// \brief A parser that gathers additional information about tokens.
 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
@@ -703,9 +718,10 @@
   Tok->Type = TT_CtorInitializerColon;
 else
   Tok->Type = TT_InheritanceColon;
-  } else if (Tok->Previous->is(tok::identifier) && Tok->Next &&
+  } else if (canBeObjCSelectorComponent(*Tok->Previous) && Tok->Next &&
  (Tok->Next->isOneOf(tok::r_paren, tok::comma) ||
-  Tok->Next->startsSequence(tok::identifier, tok::colon))) {
+  (canBeObjCSelectorComponent(*Tok->Next) && Tok->Next->Next &&
+   Tok->Next->Next->is(tok::colon {
 // This handles a special macro in ObjC code where selectors including
 // the colon are passed as macro arguments.
 Tok->Type = TT_ObjCMethodExpr;
@@ -1346,7 +1362,7 @@
  TT_LeadingJavaAnnotation)) {
 Current.Type = Current.Previous->Type;
   }
-} else if (Current.isOneOf(tok::identifier, tok::kw_new) &&
+} else if (canBeObjCSelectorComponent(Current) &&
// FIXME(bug 36976): ObjC return types shouldn't use 
TT_CastRParen.
Current.Previous && Current.Previous->is(TT_CastRParen) &&
Current.Previous->MatchingParen &&
@@ -2650,7 +2666,7 @@
   if (Line.Type == LT_ObjCMethodDecl) {
 if (Left.is(TT_ObjCMethodSpecifier))
   return true;
-if (Left.is(tok::r_paren) && Right.isOneOf(tok::identifier, tok::kw_new))
+if (Left.is(tok::r_paren) && canBeObjCSelectorComponent(Right))
   // Don't space between ')' and  or ')' and 'new'. 'new' is not a
   // keyword in Objective-C, and '+ (instancetype)new;' is a standard class
   // method declaration.
@@ -3128,6 +3144,7 @@
 for (unsigned i = 0, e = Tok->FakeLParens.size(); i != e; ++i)
   llvm::errs() << Tok->FakeLParens[i] << "/";
 llvm::errs() << " FakeRParens=" << Tok->FakeRParens;
+llvm::errs() << " II=" << Tok->Tok.getIdentifierInfo();
 llvm::errs() << " Text='" << Tok->TokenText << "'\n";
 if (!Tok->Next)
   assert(Tok == Line.Last);


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -945,15 +945,26 @@
" }]) {\n}");
 }
 
-TEST_F(FormatTestObjC, ObjCNew) {
+TEST_F(FormatTestObjC, ObjCCxxKeywords) {
   verifyFormat("+ (instancetype)new {\n"
"  return 

[PATCH] D46143: [clang-format/ObjC] Use getIdentifierInfo() instead of tok::identifier

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: djasper, jolesiak.
Herald added subscribers: cfe-commits, klimek.

Previously, we checked tokens for `tok::identifier` to see if they
were identifiers inside an Objective-C selector.

However, this missed C++ keywords like `new` and `delete`.

To fix this, this diff uses `getIdentifierInfo()` to find
identifiers or keywords inside Objective-C selectors.

Test Plan: New tests added. Ran tests with:

  % make -j16 FormatTests && ./tools/clang/unittests/Format/FormatTests


Repository:
  rC Clang

https://reviews.llvm.org/D46143

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -945,15 +945,26 @@
" }]) {\n}");
 }
 
-TEST_F(FormatTestObjC, ObjCNew) {
+TEST_F(FormatTestObjC, ObjCCxxKeywords) {
   verifyFormat("+ (instancetype)new {\n"
"  return nil;\n"
"}\n");
   verifyFormat("+ (instancetype)myNew {\n"
"  return [self new];\n"
"}\n");
   verifyFormat("SEL NewSelector(void) { return @selector(new); }\n");
   verifyFormat("SEL MacroSelector(void) { return MACRO(new); }\n");
+  verifyFormat("+ (instancetype)delete {\n"
+   "  return nil;\n"
+   "}\n");
+  verifyFormat("+ (instancetype)myDelete {\n"
+   "  return [self delete];\n"
+   "}\n");
+  verifyFormat("SEL DeleteSelector(void) { return @selector(delete); }\n");
+  verifyFormat("SEL MacroSelector(void) { return MACRO(delete); }\n");
+  verifyFormat("MACRO(new:)\n");
+  verifyFormat("MACRO(delete:)\n");
+  verifyFormat("foo = @{MACRO(new:) : MACRO(delete:)}\n");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -703,9 +703,10 @@
   Tok->Type = TT_CtorInitializerColon;
 else
   Tok->Type = TT_InheritanceColon;
-  } else if (Tok->Previous->is(tok::identifier) && Tok->Next &&
+  } else if (Tok->Previous->Tok.getIdentifierInfo() && Tok->Next &&
  (Tok->Next->isOneOf(tok::r_paren, tok::comma) ||
-  Tok->Next->startsSequence(tok::identifier, tok::colon))) {
+  (Tok->Next->Tok.getIdentifierInfo() && Tok->Next->Next &&
+   Tok->Next->Next->is(tok::colon {
 // This handles a special macro in ObjC code where selectors including
 // the colon are passed as macro arguments.
 Tok->Type = TT_ObjCMethodExpr;
@@ -1346,7 +1347,7 @@
  TT_LeadingJavaAnnotation)) {
 Current.Type = Current.Previous->Type;
   }
-} else if (Current.isOneOf(tok::identifier, tok::kw_new) &&
+} else if (Current.Tok.getIdentifierInfo() &&
// FIXME(bug 36976): ObjC return types shouldn't use 
TT_CastRParen.
Current.Previous && Current.Previous->is(TT_CastRParen) &&
Current.Previous->MatchingParen &&
@@ -2650,7 +2651,7 @@
   if (Line.Type == LT_ObjCMethodDecl) {
 if (Left.is(TT_ObjCMethodSpecifier))
   return true;
-if (Left.is(tok::r_paren) && Right.isOneOf(tok::identifier, tok::kw_new))
+if (Left.is(tok::r_paren) && Right.Tok.getIdentifierInfo())
   // Don't space between ')' and  or ')' and 'new'. 'new' is not a
   // keyword in Objective-C, and '+ (instancetype)new;' is a standard class
   // method declaration.
@@ -3128,6 +3129,7 @@
 for (unsigned i = 0, e = Tok->FakeLParens.size(); i != e; ++i)
   llvm::errs() << Tok->FakeLParens[i] << "/";
 llvm::errs() << " FakeRParens=" << Tok->FakeRParens;
+llvm::errs() << " II=" << Tok->Tok.getIdentifierInfo();
 llvm::errs() << " Text='" << Tok->TokenText << "'\n";
 if (!Tok->Next)
   assert(Tok == Line.Last);


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -945,15 +945,26 @@
" }]) {\n}");
 }
 
-TEST_F(FormatTestObjC, ObjCNew) {
+TEST_F(FormatTestObjC, ObjCCxxKeywords) {
   verifyFormat("+ (instancetype)new {\n"
"  return nil;\n"
"}\n");
   verifyFormat("+ (instancetype)myNew {\n"
"  return [self new];\n"
"}\n");
   verifyFormat("SEL NewSelector(void) { return @selector(new); }\n");
   verifyFormat("SEL MacroSelector(void) { return MACRO(new); }\n");
+  verifyFormat("+ (instancetype)delete {\n"
+   "  return nil;\n"
+   "}\n");
+  verifyFormat("+ (instancetype)myDelete {\n"
+   "  return [self delete];\n"
+   "}\n");
+