[PATCH] D46659: [clang-tidy/google-readability-casting] Allow C-style casts to/from Objective-C object types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: alexfh, Wizard, hokein.
Herald added a subscriber: cfe-commits.

Previously, `google-readability-casting` would trigger for
Objective-C++ code using C-style casts to or from Objective-C object
types.

The official Google Objective-C standard says Objective-C++ allows
authors to mix the Objective-C style (which uses C-style casts) and
C++ (which disallows it):

http://google.github.io/styleguide/objcguide.html#style-matches-the-language

So, to resolve this conflict, this diff updates
`google-readability-casting` to ignore C-style casts to or from
Objective-C object types.

Test Plan: New tests added. Ran tests with:

  % make -j16 check-clang-tools
  Before diff, confirmed tests failed:
  https://reviews.llvm.org/P8081
  After diff, confirrmed tests passed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46659

Files:
  clang-tidy/google/AvoidCStyleCastsCheck.cpp
  test/clang-tidy/google-readability-casting.mm


Index: test/clang-tidy/google-readability-casting.mm
===
--- /dev/null
+++ test/clang-tidy/google-readability-casting.mm
@@ -0,0 +1,42 @@
+// RUN: clang-tidy %s -checks=-*,google-readability-casting -- \
+// RUN:   -xobjective-c++ -fobjc-abi-version=2 -fobjc-arc | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+
+#define nil 0
+
+@interface Foo
+@end
+
+@protocol Proto
+@end
+
+@interface Bar : Foo 
+@end
+
+@interface Baz : Foo 
+@end
+
+void foo() {
+  id nilObj = nil;
+  Foo *foo = (Foo *)nilObj;
+  Bar *bar = (Bar *)nilObj;
+  id ego = (id)foo;
+  foo = (Foo *)bar;
+  foo = (id)bar;
+  id nilProto = (id)bar;
+  ego = (id)nilProto;
+  bar = (Bar *)nilProto;
+  Foo *fooProto = (Foo *)bar;
+  Baz *baz = (Baz *)bar;
+  Class klass = (Class)nilObj;
+  ego = (id)klass;
+  void *voidStar = nullptr;
+  foo = (__bridge Foo *)voidStar;
+  nilProto = (__bridge id)voidStar;
+  klass = (__bridge Class)voidStar;
+  voidStar = (__bridge void *)foo;
+  voidStar = (__bridge void *)nilProto;
+  voidStar = (__bridge void *)klass;
+}
Index: clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -110,6 +110,10 @@
   // compiled as C++.
   if (getCurrentMainFile().endswith(".c"))
 return;
+  // Ignore casts between Objective-C types.
+  if (SourceType->isObjCObjectPointerType() ||
+  DestType->isObjCObjectPointerType())
+return;
 
   SourceManager &SM = *Result.SourceManager;
 


Index: test/clang-tidy/google-readability-casting.mm
===
--- /dev/null
+++ test/clang-tidy/google-readability-casting.mm
@@ -0,0 +1,42 @@
+// RUN: clang-tidy %s -checks=-*,google-readability-casting -- \
+// RUN:   -xobjective-c++ -fobjc-abi-version=2 -fobjc-arc | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+
+#define nil 0
+
+@interface Foo
+@end
+
+@protocol Proto
+@end
+
+@interface Bar : Foo 
+@end
+
+@interface Baz : Foo 
+@end
+
+void foo() {
+  id nilObj = nil;
+  Foo *foo = (Foo *)nilObj;
+  Bar *bar = (Bar *)nilObj;
+  id ego = (id)foo;
+  foo = (Foo *)bar;
+  foo = (id)bar;
+  id nilProto = (id)bar;
+  ego = (id)nilProto;
+  bar = (Bar *)nilProto;
+  Foo *fooProto = (Foo *)bar;
+  Baz *baz = (Baz *)bar;
+  Class klass = (Class)nilObj;
+  ego = (id)klass;
+  void *voidStar = nullptr;
+  foo = (__bridge Foo *)voidStar;
+  nilProto = (__bridge id)voidStar;
+  klass = (__bridge Class)voidStar;
+  voidStar = (__bridge void *)foo;
+  voidStar = (__bridge void *)nilProto;
+  voidStar = (__bridge void *)klass;
+}
Index: clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -110,6 +110,10 @@
   // compiled as C++.
   if (getCurrentMainFile().endswith(".c"))
 return;
+  // Ignore casts between Objective-C types.
+  if (SourceType->isObjCObjectPointerType() ||
+  DestType->isObjCObjectPointerType())
+return;
 
   SourceManager &SM = *Result.SourceManager;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46659: [clang-tidy/google-readability-casting] Allow C-style casts to/from Objective-C object types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

An alternative implementation would be to allow C-style casts (either always or 
only for ObjC objects) within Objective-C methods inside Objective-C++ files, 
but that may get messy with things like shared macros.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46659



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

[PATCH] D46659: [clang-tidy/google-readability-casting] Allow C-style casts to/from Objective-C object types

[Ben Hamilton via Phabricator via cfe-commits]

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

- Change to simply disable check for Objective-C++


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46659

Files:
  clang-tidy/google/AvoidCStyleCastsCheck.cpp
  test/clang-tidy/google-readability-casting.mm

Index: test/clang-tidy/google-readability-casting.mm
===
--- /dev/null
+++ test/clang-tidy/google-readability-casting.mm
@@ -0,0 +1,179 @@
+// RUN: clang-tidy %s -checks=-*,google-readability-casting -- \
+// RUN:   -xobjective-c++ -fobjc-abi-version=2 -fobjc-arc | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+
+bool g() { return false; }
+
+enum Enum { Enum1 };
+struct X {};
+struct Y : public X {};
+
+void f(int a, double b, const char *cpc, const void *cpv, X *pX) {
+
+  typedef const char *Typedef1;
+  typedef const char *Typedef2;
+  Typedef1 t1;
+  (Typedef2)t1;
+  (const char*)t1;
+  (Typedef1)cpc;
+
+  typedef char Char;
+  char *pc;
+  Char *pChar = (Char*)pc;
+
+  (Char)*cpc;
+
+  (char)*pChar;
+
+  (const char*)cpv;
+
+  char *pc2 = (char*)(cpc + 33);
+
+  const char &crc = *cpc;
+  char &rc = (char&)crc;
+
+  char &rc2 = (char&)*cpc;
+
+  char ** const* const* ppcpcpc;
+  char c = (char)ppcpcpc;
+
+  char ***pppc = (char***)*(ppcpcpc);
+
+  char ***pppc2 = (char***)(*ppcpcpc);
+
+  char *pc5 = (char*)(const char*)(cpv);
+
+  int b1 = (int)b;
+  b1 = (const int&)b;
+
+  b1 = (int) b;
+
+  b1 = (int) b;
+
+  b1 = (int) (b);
+
+  b1 = (int) (b);
+
+  Y *pB = (Y*)pX;
+  Y &rB = (Y&)*pX;
+
+  const char *pc3 = (const char*)cpv;
+
+  char *pc4 = (char*)cpv;
+
+  b1 = (int)Enum1;
+
+  Enum e = (Enum)b1;
+
+  int b2 = int(b);
+  int b3 = static_cast(b);
+  int b4 = b;
+  double aa = a;
+  (void)b2;
+  return (void)g();
+}
+
+template 
+void template_function(T t, int n) {
+  int i = (int)t;
+}
+
+template 
+struct TemplateStruct {
+  void f(T t, int n) {
+int k = (int)t;
+  }
+};
+
+void test_templates() {
+  template_function(1, 42);
+  template_function(1.0, 42);
+  TemplateStruct().f(1, 42);
+  TemplateStruct().f(1.0, 42);
+}
+
+extern "C" {
+void extern_c_code(const char *cpc) {
+  char *pc = (char*)cpc;
+}
+}
+
+#define CAST(type, value) (type)(value)
+void macros(double d) {
+  int i = CAST(int, d);
+}
+
+enum E { E1 = 1 };
+template 
+struct A {
+  // Usage of template argument e = E1 is represented as (E)1 in the AST for
+  // some reason. We have a special treatment of this case to avoid warnings
+  // here.
+  static const E ee = e;
+};
+struct B : public A {};
+
+
+void overloaded_function();
+void overloaded_function(int);
+
+template
+void g(Fn fn) {
+  fn();
+}
+
+void function_casts() {
+  typedef void (*FnPtrVoid)();
+  typedef void (&FnRefVoid)();
+  typedef void (&FnRefInt)(int);
+
+  g((void (*)())overloaded_function);
+  g((void (*)())&overloaded_function);
+  g((void (&)())overloaded_function);
+
+  g((FnPtrVoid)overloaded_function);
+  g((FnPtrVoid)&overloaded_function);
+  g((FnRefVoid)overloaded_function);
+
+  FnPtrVoid fn0 = (void (*)())&overloaded_function;
+  FnPtrVoid fn1 = (void (*)())overloaded_function;
+  FnPtrVoid fn1a = (FnPtrVoid)overloaded_function;
+  FnRefInt fn2 = (void (&)(int))overloaded_function;
+  auto fn3 = (void (*)())&overloaded_function;
+  auto fn4 = (void (*)())overloaded_function;
+  auto fn5 = (void (&)(int))overloaded_function;
+
+  void (*fn6)() = (void (*)())&overloaded_function;
+  void (*fn7)() = (void (*)())overloaded_function;
+  void (*fn8)() = (FnPtrVoid)overloaded_function;
+  void (&fn9)(int) = (void (&)(int))overloaded_function;
+
+  void (*correct1)() = static_cast(overloaded_function);
+  FnPtrVoid correct2 = static_cast(&overloaded_function);
+  FnRefInt correct3 = static_cast(overloaded_function);
+}
+
+struct S {
+S(const char *);
+};
+struct ConvertibleToS {
+  operator S() const;
+};
+struct ConvertibleToSRef {
+  operator const S&() const;
+};
+
+void conversions() {
+  //auto s1 = (const S&)"";
+  auto s2 = (S)"";
+  auto s2a = (struct S)"";
+  auto s2b = (const S)"";
+  ConvertibleToS c;
+  auto s3 = (const S&)c;
+  auto s4 = (S)c;
+  ConvertibleToSRef cr;
+  auto s5 = (const S&)cr;
+  auto s6 = (S)cr;
+}
Index: clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -100,7 +100,8 @@
   }
 
   // The rest of this check is only relevant to C++.
-  if (!getLangOpts().CPlusPlus)
+  // We also disable it for Objective-C++.
+  if (!getLangOpts().CPlusPlus || getLangOpts().ObjC1 || getLangOpts().ObjC2)
 return;
   // Ignore code inside extern "C" {} blocks.
   if (!match(expr(hasAncestor(linkageSpecDecl())), *CastExpr, *Result.Context)
___
cfe-commits mailing list

[PATCH] D46659: [clang-tidy/google-readability-casting] Disable check for Objective-C++

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

After discussion, I changed this diff to simply disable the check altogether 
for Objective-C++.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46659



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

[PATCH] D46659: [clang-tidy/google-readability-casting] Disable check for Objective-C++

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: clang-tidy/google/AvoidCStyleCastsCheck.cpp:103-104
   // The rest of this check is only relevant to C++.
-  if (!getLangOpts().CPlusPlus)
+  // We also disable it for Objective-C++.
+  if (!getLangOpts().CPlusPlus || getLangOpts().ObjC1 || getLangOpts().ObjC2)
 return;

stephanemoore wrote:
> In the future it would probably be good to allow configuring whether or not 
> this is disabled but I think that disabling it for Objective-C++ completely 
> in the interim is a positive change.
Agreed. Filed https://bugs.llvm.org/show_bug.cgi?id=37489 to follow up.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46659



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

[PATCH] D46659: [clang-tidy/google-readability-casting] Disable check for Objective-C++

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL332516: [clang-tidy/google-readability-casting] Disable 
check for Objective-C++ (authored by benhamilton, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D46659

Files:
  clang-tools-extra/trunk/clang-tidy/google/AvoidCStyleCastsCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/google-readability-casting.mm

Index: clang-tools-extra/trunk/clang-tidy/google/AvoidCStyleCastsCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/google/AvoidCStyleCastsCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/google/AvoidCStyleCastsCheck.cpp
@@ -100,7 +100,8 @@
   }
 
   // The rest of this check is only relevant to C++.
-  if (!getLangOpts().CPlusPlus)
+  // We also disable it for Objective-C++.
+  if (!getLangOpts().CPlusPlus || getLangOpts().ObjC1 || getLangOpts().ObjC2)
 return;
   // Ignore code inside extern "C" {} blocks.
   if (!match(expr(hasAncestor(linkageSpecDecl())), *CastExpr, *Result.Context)
Index: clang-tools-extra/trunk/test/clang-tidy/google-readability-casting.mm
===
--- clang-tools-extra/trunk/test/clang-tidy/google-readability-casting.mm
+++ clang-tools-extra/trunk/test/clang-tidy/google-readability-casting.mm
@@ -0,0 +1,179 @@
+// RUN: clang-tidy %s -checks=-*,google-readability-casting -- \
+// RUN:   -xobjective-c++ -fobjc-abi-version=2 -fobjc-arc | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+
+bool g() { return false; }
+
+enum Enum { Enum1 };
+struct X {};
+struct Y : public X {};
+
+void f(int a, double b, const char *cpc, const void *cpv, X *pX) {
+
+  typedef const char *Typedef1;
+  typedef const char *Typedef2;
+  Typedef1 t1;
+  (Typedef2)t1;
+  (const char*)t1;
+  (Typedef1)cpc;
+
+  typedef char Char;
+  char *pc;
+  Char *pChar = (Char*)pc;
+
+  (Char)*cpc;
+
+  (char)*pChar;
+
+  (const char*)cpv;
+
+  char *pc2 = (char*)(cpc + 33);
+
+  const char &crc = *cpc;
+  char &rc = (char&)crc;
+
+  char &rc2 = (char&)*cpc;
+
+  char ** const* const* ppcpcpc;
+  char c = (char)ppcpcpc;
+
+  char ***pppc = (char***)*(ppcpcpc);
+
+  char ***pppc2 = (char***)(*ppcpcpc);
+
+  char *pc5 = (char*)(const char*)(cpv);
+
+  int b1 = (int)b;
+  b1 = (const int&)b;
+
+  b1 = (int) b;
+
+  b1 = (int) b;
+
+  b1 = (int) (b);
+
+  b1 = (int) (b);
+
+  Y *pB = (Y*)pX;
+  Y &rB = (Y&)*pX;
+
+  const char *pc3 = (const char*)cpv;
+
+  char *pc4 = (char*)cpv;
+
+  b1 = (int)Enum1;
+
+  Enum e = (Enum)b1;
+
+  int b2 = int(b);
+  int b3 = static_cast(b);
+  int b4 = b;
+  double aa = a;
+  (void)b2;
+  return (void)g();
+}
+
+template 
+void template_function(T t, int n) {
+  int i = (int)t;
+}
+
+template 
+struct TemplateStruct {
+  void f(T t, int n) {
+int k = (int)t;
+  }
+};
+
+void test_templates() {
+  template_function(1, 42);
+  template_function(1.0, 42);
+  TemplateStruct().f(1, 42);
+  TemplateStruct().f(1.0, 42);
+}
+
+extern "C" {
+void extern_c_code(const char *cpc) {
+  char *pc = (char*)cpc;
+}
+}
+
+#define CAST(type, value) (type)(value)
+void macros(double d) {
+  int i = CAST(int, d);
+}
+
+enum E { E1 = 1 };
+template 
+struct A {
+  // Usage of template argument e = E1 is represented as (E)1 in the AST for
+  // some reason. We have a special treatment of this case to avoid warnings
+  // here.
+  static const E ee = e;
+};
+struct B : public A {};
+
+
+void overloaded_function();
+void overloaded_function(int);
+
+template
+void g(Fn fn) {
+  fn();
+}
+
+void function_casts() {
+  typedef void (*FnPtrVoid)();
+  typedef void (&FnRefVoid)();
+  typedef void (&FnRefInt)(int);
+
+  g((void (*)())overloaded_function);
+  g((void (*)())&overloaded_function);
+  g((void (&)())overloaded_function);
+
+  g((FnPtrVoid)overloaded_function);
+  g((FnPtrVoid)&overloaded_function);
+  g((FnRefVoid)overloaded_function);
+
+  FnPtrVoid fn0 = (void (*)())&overloaded_function;
+  FnPtrVoid fn1 = (void (*)())overloaded_function;
+  FnPtrVoid fn1a = (FnPtrVoid)overloaded_function;
+  FnRefInt fn2 = (void (&)(int))overloaded_function;
+  auto fn3 = (void (*)())&overloaded_function;
+  auto fn4 = (void (*)())overloaded_function;
+  auto fn5 = (void (&)(int))overloaded_function;
+
+  void (*fn6)() = (void (*)())&overloaded_function;
+  void (*fn7)() = (void (*)())overloaded_function;
+  void (*fn8)() = (FnPtrVoid)overloaded_function;
+  void (&fn9)(int) = (void (&)(int))overloaded_function;
+
+  void (*correct1)() = static_cast(overloaded_function);
+  FnPtrVoid correct2 = static_cast(&overloaded_function);
+  FnRefInt correct3 = static_cast(overloaded_function);
+}
+
+struct S {
+S(const char *);
+};
+struct ConvertibleToS {
+  operator S() const;
+};
+struct ConvertibleToSRef {
+  operator const S&() const;
+};
+
+

[PATCH] D47028: [clang-format/ObjC] Correctly annotate single-component ObjC method invocations

[Ben Hamilton via Phabricator via cfe-commits]

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

Previously, clang-format's parser would fail to annotate the
selector in a single-component Objective-C method invocation with
`TT_SelectorName`. For example, the following:

  [foo bar];

would parse `bar` as `TT_Unknown`:

  M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=34 PPK=2
  FakeLParens= FakeRParens=0 II=0x559d5db51770 Text='bar'

This caused us to fail to insert a space after a closing cast rparen,
so the following:

  [((Foo *)foo) bar];

would format as:

  [((Foo *)foo)bar];

This diff fixes the issue by ensuring we annotate the selector
in a single-component Objective-C method invocation as
`TT_SelectorName`.

Test Plan: New tests added. Ran tests with:

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


Repository:
  rC Clang

https://reviews.llvm.org/D47028

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -792,6 +792,10 @@
"  a = 42;\n"
"}];");
 
+  // Space between cast rparen and selector name component.
+  verifyFormat("[((Foo *)foo) bar];");
+  verifyFormat("[((Foo *)foo) bar:1 blech:2];");
+
   // Message receiver taking multiple lines.
   Style.ColumnLimit = 20;
   // Non-corner case.
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -501,6 +501,13 @@
 }
 if (StartsObjCMethodExpr && CurrentToken->Previous != Left) {
   CurrentToken->Type = TT_ObjCMethodExpr;
+  // If we haven't seen a colon yet, make sure the last identifier
+  // before the r_square is tagged as a selector name component.
+  if (!ColonFound && CurrentToken->Previous &&
+  CurrentToken->Previous->is(TT_Unknown) &&
+  canBeObjCSelectorComponent(*CurrentToken->Previous)) {
+CurrentToken->Previous->Type = TT_SelectorName;
+  }
   // determineStarAmpUsage() thinks that '*' '[' is allocating an
   // array of pointers, but if '[' starts a selector then '*' is a
   // binary operator.


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -792,6 +792,10 @@
"  a = 42;\n"
"}];");
 
+  // Space between cast rparen and selector name component.
+  verifyFormat("[((Foo *)foo) bar];");
+  verifyFormat("[((Foo *)foo) bar:1 blech:2];");
+
   // Message receiver taking multiple lines.
   Style.ColumnLimit = 20;
   // Non-corner case.
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -501,6 +501,13 @@
 }
 if (StartsObjCMethodExpr && CurrentToken->Previous != Left) {
   CurrentToken->Type = TT_ObjCMethodExpr;
+  // If we haven't seen a colon yet, make sure the last identifier
+  // before the r_square is tagged as a selector name component.
+  if (!ColonFound && CurrentToken->Previous &&
+  CurrentToken->Previous->is(TT_Unknown) &&
+  canBeObjCSelectorComponent(*CurrentToken->Previous)) {
+CurrentToken->Previous->Type = TT_SelectorName;
+  }
   // determineStarAmpUsage() thinks that '*' '[' is allocating an
   // array of pointers, but if '[' starts a selector then '*' is a
   // binary operator.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47028: [clang-format/ObjC] Correctly annotate single-component ObjC method invocations

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:510
+CurrentToken->Previous->Type = TT_SelectorName;
+  }
   // determineStarAmpUsage() thinks that '*' '[' is allocating an

jolesiak wrote:
> I'd remove braces for consistency.
Thanks, done.


Repository:
  rC Clang

https://reviews.llvm.org/D47028



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

[PATCH] D47028: [clang-format/ObjC] Correctly annotate single-component ObjC method invocations

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 147516.
benhamilton marked an inline comment as done.
benhamilton added a comment.

Style


Repository:
  rC Clang

https://reviews.llvm.org/D47028

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -792,6 +792,10 @@
"  a = 42;\n"
"}];");
 
+  // Space between cast rparen and selector name component.
+  verifyFormat("[((Foo *)foo) bar];");
+  verifyFormat("[((Foo *)foo) bar:1 blech:2];");
+
   // Message receiver taking multiple lines.
   Style.ColumnLimit = 20;
   // Non-corner case.
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -501,6 +501,12 @@
 }
 if (StartsObjCMethodExpr && CurrentToken->Previous != Left) {
   CurrentToken->Type = TT_ObjCMethodExpr;
+  // If we haven't seen a colon yet, make sure the last identifier
+  // before the r_square is tagged as a selector name component.
+  if (!ColonFound && CurrentToken->Previous &&
+  CurrentToken->Previous->is(TT_Unknown) &&
+  canBeObjCSelectorComponent(*CurrentToken->Previous))
+CurrentToken->Previous->Type = TT_SelectorName;
   // determineStarAmpUsage() thinks that '*' '[' is allocating an
   // array of pointers, but if '[' starts a selector then '*' is a
   // binary operator.


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -792,6 +792,10 @@
"  a = 42;\n"
"}];");
 
+  // Space between cast rparen and selector name component.
+  verifyFormat("[((Foo *)foo) bar];");
+  verifyFormat("[((Foo *)foo) bar:1 blech:2];");
+
   // Message receiver taking multiple lines.
   Style.ColumnLimit = 20;
   // Non-corner case.
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -501,6 +501,12 @@
 }
 if (StartsObjCMethodExpr && CurrentToken->Previous != Left) {
   CurrentToken->Type = TT_ObjCMethodExpr;
+  // If we haven't seen a colon yet, make sure the last identifier
+  // before the r_square is tagged as a selector name component.
+  if (!ColonFound && CurrentToken->Previous &&
+  CurrentToken->Previous->is(TT_Unknown) &&
+  canBeObjCSelectorComponent(*CurrentToken->Previous))
+CurrentToken->Previous->Type = TT_SelectorName;
   // determineStarAmpUsage() thinks that '*' '[' is allocating an
   // array of pointers, but if '[' starts a selector then '*' is a
   // binary operator.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47028: [clang-format/ObjC] Correctly annotate single-component ObjC method invocations

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC332727: [clang-format/ObjC] Correctly annotate 
single-component ObjC method invocations (authored by benhamilton, committed by 
).

Changed prior to commit:
  https://reviews.llvm.org/D47028?vs=147516&id=147517#toc

Repository:
  rC Clang

https://reviews.llvm.org/D47028

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


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -501,6 +501,12 @@
 }
 if (StartsObjCMethodExpr && CurrentToken->Previous != Left) {
   CurrentToken->Type = TT_ObjCMethodExpr;
+  // If we haven't seen a colon yet, make sure the last identifier
+  // before the r_square is tagged as a selector name component.
+  if (!ColonFound && CurrentToken->Previous &&
+  CurrentToken->Previous->is(TT_Unknown) &&
+  canBeObjCSelectorComponent(*CurrentToken->Previous))
+CurrentToken->Previous->Type = TT_SelectorName;
   // determineStarAmpUsage() thinks that '*' '[' is allocating an
   // array of pointers, but if '[' starts a selector then '*' is a
   // binary operator.
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -792,6 +792,10 @@
"  a = 42;\n"
"}];");
 
+  // Space between cast rparen and selector name component.
+  verifyFormat("[((Foo *)foo) bar];");
+  verifyFormat("[((Foo *)foo) bar:1 blech:2];");
+
   // Message receiver taking multiple lines.
   Style.ColumnLimit = 20;
   // Non-corner case.


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -501,6 +501,12 @@
 }
 if (StartsObjCMethodExpr && CurrentToken->Previous != Left) {
   CurrentToken->Type = TT_ObjCMethodExpr;
+  // If we haven't seen a colon yet, make sure the last identifier
+  // before the r_square is tagged as a selector name component.
+  if (!ColonFound && CurrentToken->Previous &&
+  CurrentToken->Previous->is(TT_Unknown) &&
+  canBeObjCSelectorComponent(*CurrentToken->Previous))
+CurrentToken->Previous->Type = TT_SelectorName;
   // determineStarAmpUsage() thinks that '*' '[' is allocating an
   // array of pointers, but if '[' starts a selector then '*' is a
   // binary operator.
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -792,6 +792,10 @@
"  a = 42;\n"
"}];");
 
+  // Space between cast rparen and selector name component.
+  verifyFormat("[((Foo *)foo) bar];");
+  verifyFormat("[((Foo *)foo) bar:1 blech:2];");
+
   // Message receiver taking multiple lines.
   Style.ColumnLimit = 20;
   // Non-corner case.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47095: [clang-format/ObjC] Correctly parse Objective-C methods with 'class' in name

[Ben Hamilton via Phabricator via cfe-commits]

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

Please take a close look at this CL. I haven't touched much of
`UnwrappedLineParser` before, so I may have gotten things wrong.

Previously, clang-format would incorrectly format the following:

  @implementation Foo
  
  - (Class)class {
  }
  
  - (void)foo {
  }
  
  @end

as:

  @implementation Foo
  
  - (Class)class {
  }
  
  - (void)foo {
  }
  
  @end

The problem is whenever `UnwrappedLineParser::parseStructuralElement()`
sees any of the keywords `class`, `struct`, or `enum`, it calls
`parseRecord()` to parse them as a C/C++ record.

This causes subsequent lines to be parsed incorrectly, which
causes them to be indented incorrectly.

In Objective-C/Objective-C++, these keywords are valid selector
components.

This diff fixes the issue by explicitly handling `+` and `-` lines
inside `@implementation` / `@interface` / `@protocol` blocks
and parsing them as Objective-C methods.

Test Plan: New tests added. Ran tests with:

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


Repository:
  rC Clang

https://reviews.llvm.org/D47095

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -998,6 +998,59 @@
   verifyFormat("MACRO(new:)\n");
   verifyFormat("MACRO(delete:)\n");
   verifyFormat("foo = @{MACRO(new:) : MACRO(delete:)}\n");
+  verifyFormat("@implementation Foo\n"
+   "// Testing\n"
+   "- (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end\n");
+  verifyFormat("@implementation Foo\n"
+   "- (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "+ (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "- (Class)class:(Class)klass {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "+ (Class)class:(Class)klass {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+
+  verifyFormat("@interface Foo\n"
+   "// Testing\n"
+   "- (Class)class;\n"
+   "- (void)foo;\n"
+   "@end\n");
+  verifyFormat("@interface Foo\n"
+   "- (Class)class;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "+ (Class)class;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "- (Class)class:(Class)klass;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "+ (Class)class:(Class)klass;\n"
+   "- (void)foo;\n"
+   "@end");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -120,6 +120,7 @@
   // parses the record as a child block, i.e. if the class declaration is an
   // expression.
   void parseRecord(bool ParseAsExpr = false);
+  void parseObjCMethod();
   void parseObjCProtocolList();
   void parseObjCUntilAtEnd();
   void parseObjCInterfaceOrImplementation();
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2130,6 +2130,22 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+void UnwrappedLineParser::parseObjCMethod() {
+  assert(FormatTok->Tok.isOneOf(tok::l_paren, tok::identifier) && "'(' or identifier expected.");
+  do {
+if (FormatTok->Tok.isOneOf(tok::semi, tok::r_brace)) {
+  nextToken();
+  addUnwrappedLine();
+  return;
+} else if (FormatTok->Tok.is(tok::l_brace)) {
+  parseBlock(/*MustBeDeclaration=*/false);
+  addUnwrappedLine();
+} else {
+  nextToken();
+}
+  } while (!eof());
+}
+
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
   do {
@@ -2157,6 +2173,9 @@
   // Ignore stray "}". parseStructuralElement doesn't consume them.
   nextToken();
   addUnwrappedLine();
+} else if (FormatTok->isOneOf(tok::minus, tok::plus)) {
+  nextToken();
+  parseObjCMethod();
 } else {
   parseStruct

[PATCH] D47095: [clang-format/ObjC] Correctly parse Objective-C methods with 'class' in name

[Ben Hamilton via Phabricator via cfe-commits]

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

Format


Repository:
  rC Clang

https://reviews.llvm.org/D47095

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -998,6 +998,59 @@
   verifyFormat("MACRO(new:)\n");
   verifyFormat("MACRO(delete:)\n");
   verifyFormat("foo = @{MACRO(new:) : MACRO(delete:)}\n");
+  verifyFormat("@implementation Foo\n"
+   "// Testing\n"
+   "- (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end\n");
+  verifyFormat("@implementation Foo\n"
+   "- (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "+ (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "- (Class)class:(Class)klass {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "+ (Class)class:(Class)klass {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+
+  verifyFormat("@interface Foo\n"
+   "// Testing\n"
+   "- (Class)class;\n"
+   "- (void)foo;\n"
+   "@end\n");
+  verifyFormat("@interface Foo\n"
+   "- (Class)class;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "+ (Class)class;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "- (Class)class:(Class)klass;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "+ (Class)class:(Class)klass;\n"
+   "- (void)foo;\n"
+   "@end");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -120,6 +120,7 @@
   // parses the record as a child block, i.e. if the class declaration is an
   // expression.
   void parseRecord(bool ParseAsExpr = false);
+  void parseObjCMethod();
   void parseObjCProtocolList();
   void parseObjCUntilAtEnd();
   void parseObjCInterfaceOrImplementation();
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2130,6 +2130,23 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+void UnwrappedLineParser::parseObjCMethod() {
+  assert(FormatTok->Tok.isOneOf(tok::l_paren, tok::identifier) &&
+ "'(' or identifier expected.");
+  do {
+if (FormatTok->Tok.isOneOf(tok::semi, tok::r_brace)) {
+  nextToken();
+  addUnwrappedLine();
+  return;
+} else if (FormatTok->Tok.is(tok::l_brace)) {
+  parseBlock(/*MustBeDeclaration=*/false);
+  addUnwrappedLine();
+} else {
+  nextToken();
+}
+  } while (!eof());
+}
+
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
   do {
@@ -2157,6 +2174,9 @@
   // Ignore stray "}". parseStructuralElement doesn't consume them.
   nextToken();
   addUnwrappedLine();
+} else if (FormatTok->isOneOf(tok::minus, tok::plus)) {
+  nextToken();
+  parseObjCMethod();
 } else {
   parseStructuralElement();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47195: [clang-format] Fix ObjC message arguments handling

[Ben Hamilton via Phabricator via cfe-commits]

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

Please address @krasimir's comments, but looks good to me.


Repository:
  rC Clang

https://reviews.llvm.org/D47195



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

[PATCH] D47195: [clang-format] Fix ObjC message arguments handling

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Can you update the diff description to reflect that it includes the original 
change?


Repository:
  rC Clang

https://reviews.llvm.org/D47195



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

[PATCH] D40221: [clang-format] Parse blocks in braced lists

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton edited reviewers, added: klimek; removed: djasper.
benhamilton added a comment.

@djasper isn't available to review.

At a high level, this seems good, but I'd like @klimek to take a look.




Comment at: lib/Format/UnwrappedLineParser.cpp:1320
+// \endcode
+bool UnwrappedLineParser::tryToParseBlock() {
+  // Consume the leading ^.

Is it standard to return a value from these `tryToParseFoo()` methods, even if 
nobody uses it?

I think we should either check the return value somewhere, or make this return 
`void`.



Comment at: lib/Format/UnwrappedLineParser.cpp:1324-1327
+  if (!Style.isCpp()) {
+// Blocks are only supported in C++ and Objective-C.
+return false;
+  }

Style: Remove curly braces for one-line if blocks.



https://reviews.llvm.org/D40221



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

[PATCH] D47095: [clang-format/ObjC] Correctly parse Objective-C methods with 'class' in name

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:2137
+  do {
+if (FormatTok->Tok.isOneOf(tok::semi, tok::r_brace)) {
+  nextToken();

jolesiak wrote:
> `tok::r_brace` could be skiped - see comment to line 2143.
Done.



Comment at: lib/Format/UnwrappedLineParser.cpp:2143
+  parseBlock(/*MustBeDeclaration=*/false);
+  addUnwrappedLine();
+} else {

jolesiak wrote:
> We have to add `return` after `addUnwrappedLine` as `parseBlock` does consume 
> `tok::r_brace`. Without `return` we will consume tokens after `}`. This 
> problem will rarely occur as most lines end with `tok::semi` or 
> `tok::r_brace` and it will be terminated properly (however maybe not handled 
> properly as we just skip every token in `else`) by `if` branch.
> 
> Test like:
> ```
> @implementation Foo
> - (foo)foo {
> }
> @end
> @implementation Bar
> - (bar)bar {
> }
> @end
> ```
> will distinguish version with `return` from one without. Therefore, I think 
> we should add it.
Done, test added.


Repository:
  rC Clang

https://reviews.llvm.org/D47095



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

[PATCH] D47095: [clang-format/ObjC] Correctly parse Objective-C methods with 'class' in name

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 148231.
benhamilton marked 2 inline comments as done.
benhamilton added a comment.

@jolesiak


Repository:
  rC Clang

https://reviews.llvm.org/D47095

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -328,7 +328,14 @@
"}\n"
"+ (id)init;\n"
"@end");
-
+  verifyFormat("@interface Foo\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end\n"
+   "@implementation Bar\n"
+   "- (void)bar {\n"
+   "}\n"
+   "@end");
   Style.ColumnLimit = 40;
   verifyFormat("@interface c () <\n"
"c, c,\n"
@@ -998,6 +1005,59 @@
   verifyFormat("MACRO(new:)\n");
   verifyFormat("MACRO(delete:)\n");
   verifyFormat("foo = @{MACRO(new:) : MACRO(delete:)}\n");
+  verifyFormat("@implementation Foo\n"
+   "// Testing\n"
+   "- (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end\n");
+  verifyFormat("@implementation Foo\n"
+   "- (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "+ (Class)class {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "- (Class)class:(Class)klass {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+  verifyFormat("@implementation Foo\n"
+   "+ (Class)class:(Class)klass {\n"
+   "}\n"
+   "- (void)foo {\n"
+   "}\n"
+   "@end");
+
+  verifyFormat("@interface Foo\n"
+   "// Testing\n"
+   "- (Class)class;\n"
+   "- (void)foo;\n"
+   "@end\n");
+  verifyFormat("@interface Foo\n"
+   "- (Class)class;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "+ (Class)class;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "- (Class)class:(Class)klass;\n"
+   "- (void)foo;\n"
+   "@end");
+  verifyFormat("@interface Foo\n"
+   "+ (Class)class:(Class)klass;\n"
+   "- (void)foo;\n"
+   "@end");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -120,6 +120,7 @@
   // parses the record as a child block, i.e. if the class declaration is an
   // expression.
   void parseRecord(bool ParseAsExpr = false);
+  void parseObjCMethod();
   void parseObjCProtocolList();
   void parseObjCUntilAtEnd();
   void parseObjCInterfaceOrImplementation();
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2130,6 +2130,24 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+void UnwrappedLineParser::parseObjCMethod() {
+  assert(FormatTok->Tok.isOneOf(tok::l_paren, tok::identifier) &&
+ "'(' or identifier expected.");
+  do {
+if (FormatTok->Tok.is(tok::semi)) {
+  nextToken();
+  addUnwrappedLine();
+  return;
+} else if (FormatTok->Tok.is(tok::l_brace)) {
+  parseBlock(/*MustBeDeclaration=*/false);
+  addUnwrappedLine();
+  return;
+} else {
+  nextToken();
+}
+  } while (!eof());
+}
+
 void UnwrappedLineParser::parseObjCProtocolList() {
   assert(FormatTok->Tok.is(tok::less) && "'<' expected.");
   do {
@@ -2157,6 +2175,9 @@
   // Ignore stray "}". parseStructuralElement doesn't consume them.
   nextToken();
   addUnwrappedLine();
+} else if (FormatTok->isOneOf(tok::minus, tok::plus)) {
+  nextToken();
+  parseObjCMethod();
 } else {
   parseStructuralElement();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51575: [clang-tidy] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton requested changes to this revision.
benhamilton added a comment.
This revision now requires changes to proceed.

Thanks for this! Let's consolidate this with the property name checker (either 
simplify the logic there and allow `arBiTRAryCapSAnYWHere` or apply the same 
registered acronym logic here).




Comment at: clang-tidy/google/FunctionNamingCheck.cpp:35
+  // non-standard capitalized character sequences including acronyms,
+  // initialisms, and prefixes of symbols (e.g., UIColorFromNSString). For this
+  // reason, the regex only verifies that the function name after the prefix

Any reason why this is different from the implementation in the property name 
checker? Either we should allow both of:

```
void ARBiTraRilyNameDFuncTioN();
// ...
@property (...) id arBItrArIlyNameD;
```

or we should require that acronyms in the middle of the name be 
registered/known acronyms for both properties and functions.

I believe this diff allows arbitrary capitalization for functions, but we 
disallowed that for property names, so I think we should be consistent.



Comment at: clang-tidy/google/FunctionNamingCheck.cpp:50
+
+void FunctionNamingCheck::registerMatchers(MatchFinder *Finder) {
+  // This check should only be applied to Objective-C sources.

Wizard wrote:
> Can we do some simple check to see if some easy fix can be provided just like 
> `objc-property-declaration` check?
> Something like `static bool isPositive` to `static bool IsPositive` and 
> `static bool is_upper_camel` to `IsUpperCamel`. Such check can help provide 
> code fix for a lot of  very common mistake at a low cost (i.e. if the naming 
> pattern cannot be simply recognized, just provide no fix).
+1, I think the two checks should be substantially similar.



Comment at: clang-tidy/google/FunctionNamingCheck.h:21
+
+/// Finds function names that do not conform to the recommendations of the
+/// Google Objective-C Style Guide. Function names should be in upper camel 
case

Worth mentioning this does not apply to Objective-C method names, nor 
Objective-C properties.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51575



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

[PATCH] D51575: [clang-tidy] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: clang-tidy/google/FunctionNamingCheck.cpp:35
+  // non-standard capitalized character sequences including acronyms,
+  // initialisms, and prefixes of symbols (e.g., UIColorFromNSString). For this
+  // reason, the regex only verifies that the function name after the prefix

benhamilton wrote:
> Any reason why this is different from the implementation in the property name 
> checker? Either we should allow both of:
> 
> ```
> void ARBiTraRilyNameDFuncTioN();
> // ...
> @property (...) id arBItrArIlyNameD;
> ```
> 
> or we should require that acronyms in the middle of the name be 
> registered/known acronyms for both properties and functions.
> 
> I believe this diff allows arbitrary capitalization for functions, but we 
> disallowed that for property names, so I think we should be consistent.
(And, just to be clear, I don't feel strongly which direction we go — it's 
certainly less work to allow arbitrarily-named functions and properties than to 
maintain the acronym list.)



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51575



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

[PATCH] D51819: [clang-tidy/ObjC] Update list of acronyms in PropertyDeclarationCheck

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: Wizard, hokein.
Herald added a subscriber: cfe-commits.

This adds a few common acronyms we found were missing from 
PropertyDeclarationCheck.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51819

Files:
  clang-tidy/objc/PropertyDeclarationCheck.cpp


Index: clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -42,12 +42,15 @@
 "[2-9]G",
 "ACL",
 "API",
+"APN",
+"APNS",
 "AR",
 "ARGB",
 "ASCII",
 "AV",
 "BGRA",
 "CA",
+"CDN",
 "CF",
 "CG",
 "CI",
@@ -71,14 +74,17 @@
 "ID",
 "JPG",
 "JS",
+"JSON",
 "LAN",
 "LZW",
+"LTR",
 "MAC",
 "MD",
 "MDNS",
 "MIDI",
 "NS",
 "OS",
+"P2P",
 "PDF",
 "PIN",
 "PNG",
@@ -102,12 +108,14 @@
 "SSO",
 "TCP",
 "TIFF",
+"TOS",
 "TTS",
 "UI",
 "URI",
 "URL",
 "UUID",
 "VC",
+"VO",
 "VOIP",
 "VPN",
 "VR",


Index: clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -42,12 +42,15 @@
 "[2-9]G",
 "ACL",
 "API",
+"APN",
+"APNS",
 "AR",
 "ARGB",
 "ASCII",
 "AV",
 "BGRA",
 "CA",
+"CDN",
 "CF",
 "CG",
 "CI",
@@ -71,14 +74,17 @@
 "ID",
 "JPG",
 "JS",
+"JSON",
 "LAN",
 "LZW",
+"LTR",
 "MAC",
 "MD",
 "MDNS",
 "MIDI",
 "NS",
 "OS",
+"P2P",
 "PDF",
 "PIN",
 "PNG",
@@ -102,12 +108,14 @@
 "SSO",
 "TCP",
 "TIFF",
+"TOS",
 "TTS",
 "UI",
 "URI",
 "URL",
 "UUID",
 "VC",
+"VO",
 "VOIP",
 "VPN",
 "VR",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51819: [clang-tidy/ObjC] Update list of acronyms in PropertyDeclarationCheck

[Ben Hamilton via Phabricator via cfe-commits]

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

- Update docs.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51819

Files:
  clang-tidy/objc/PropertyDeclarationCheck.cpp
  docs/clang-tidy/checks/objc-property-declaration.rst


Index: docs/clang-tidy/checks/objc-property-declaration.rst
===
--- docs/clang-tidy/checks/objc-property-declaration.rst
+++ docs/clang-tidy/checks/objc-property-declaration.rst
@@ -63,7 +63,7 @@
If set to ``1``, the value in ``Acronyms`` is appended to the
default list of acronyms:
 
-   
``ACL;API;ARGB;ASCII;BGRA;CMYK;DNS;FPS;FTP;GIF;GPS;HD;HDR;HTML;HTTP;HTTPS;HUD;ID;JPG;JS;LAN;LZW;MDNS;MIDI;OS;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;RGBX;ROM;RPC;RTF;RTL;SDK;SSO;TCP;TIFF;TTS;UI;URI;URL;VC;VOIP;VPN;VR;WAN;XML``.
+   
``[2-9]G;ACL;API;APN;APNS;AR;ARGB;ASCII;AV;BGRA;CA;CDN;CF;CG;CI;CRC;CV;CMYK;DNS;FPS;FTP;GIF;GL;GPS;GUID;HD;HDR;HMAC;HTML;HTTP;HTTPS;HUD;ID;JPG;JS;JSON;LAN;LZW;LTR;MAC;MD;MDNS;MIDI;NS;OS;P2P;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;RGBX;RIPEMD;ROM;RPC;RTF;RTL;SC;SDK;SHA;SQL;SSO;TCP;TIFF;TOS;TTS;UI;URI;URL;UUID;VC;VO;VOIP;VPN;VR;W;WAN;X;XML;Y;Z``.
 
If set to ``0``, the value in ``Acronyms`` replaces the default list
of acronyms.
Index: clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -42,12 +42,15 @@
 "[2-9]G",
 "ACL",
 "API",
+"APN",
+"APNS",
 "AR",
 "ARGB",
 "ASCII",
 "AV",
 "BGRA",
 "CA",
+"CDN",
 "CF",
 "CG",
 "CI",
@@ -71,14 +74,17 @@
 "ID",
 "JPG",
 "JS",
+"JSON",
 "LAN",
 "LZW",
+"LTR",
 "MAC",
 "MD",
 "MDNS",
 "MIDI",
 "NS",
 "OS",
+"P2P",
 "PDF",
 "PIN",
 "PNG",
@@ -102,12 +108,14 @@
 "SSO",
 "TCP",
 "TIFF",
+"TOS",
 "TTS",
 "UI",
 "URI",
 "URL",
 "UUID",
 "VC",
+"VO",
 "VOIP",
 "VPN",
 "VR",


Index: docs/clang-tidy/checks/objc-property-declaration.rst
===
--- docs/clang-tidy/checks/objc-property-declaration.rst
+++ docs/clang-tidy/checks/objc-property-declaration.rst
@@ -63,7 +63,7 @@
If set to ``1``, the value in ``Acronyms`` is appended to the
default list of acronyms:
 
-   ``ACL;API;ARGB;ASCII;BGRA;CMYK;DNS;FPS;FTP;GIF;GPS;HD;HDR;HTML;HTTP;HTTPS;HUD;ID;JPG;JS;LAN;LZW;MDNS;MIDI;OS;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;RGBX;ROM;RPC;RTF;RTL;SDK;SSO;TCP;TIFF;TTS;UI;URI;URL;VC;VOIP;VPN;VR;WAN;XML``.
+   ``[2-9]G;ACL;API;APN;APNS;AR;ARGB;ASCII;AV;BGRA;CA;CDN;CF;CG;CI;CRC;CV;CMYK;DNS;FPS;FTP;GIF;GL;GPS;GUID;HD;HDR;HMAC;HTML;HTTP;HTTPS;HUD;ID;JPG;JS;JSON;LAN;LZW;LTR;MAC;MD;MDNS;MIDI;NS;OS;P2P;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;RGBX;RIPEMD;ROM;RPC;RTF;RTL;SC;SDK;SHA;SQL;SSO;TCP;TIFF;TOS;TTS;UI;URI;URL;UUID;VC;VO;VOIP;VPN;VR;W;WAN;X;XML;Y;Z``.
 
If set to ``0``, the value in ``Acronyms`` replaces the default list
of acronyms.
Index: clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -42,12 +42,15 @@
 "[2-9]G",
 "ACL",
 "API",
+"APN",
+"APNS",
 "AR",
 "ARGB",
 "ASCII",
 "AV",
 "BGRA",
 "CA",
+"CDN",
 "CF",
 "CG",
 "CI",
@@ -71,14 +74,17 @@
 "ID",
 "JPG",
 "JS",
+"JSON",
 "LAN",
 "LZW",
+"LTR",
 "MAC",
 "MD",
 "MDNS",
 "MIDI",
 "NS",
 "OS",
+"P2P",
 "PDF",
 "PIN",
 "PNG",
@@ -102,12 +108,14 @@
 "SSO",
 "TCP",
 "TIFF",
+"TOS",
 "TTS",
 "UI",
 "URI",
 "URL",
 "UUID",
 "VC",
+"VO",
 "VOIP",
 "VPN",
 "VR",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51819: [clang-tidy/ObjC] Update list of acronyms in PropertyDeclarationCheck

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL341720: [clang-tidy/ObjC] Update list of acronyms in 
PropertyDeclarationCheck (authored by benhamilton, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51819?vs=164523&id=164525#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51819

Files:
  clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp


Index: clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -42,12 +42,15 @@
 "[2-9]G",
 "ACL",
 "API",
+"APN",
+"APNS",
 "AR",
 "ARGB",
 "ASCII",
 "AV",
 "BGRA",
 "CA",
+"CDN",
 "CF",
 "CG",
 "CI",
@@ -71,14 +74,17 @@
 "ID",
 "JPG",
 "JS",
+"JSON",
 "LAN",
 "LZW",
+"LTR",
 "MAC",
 "MD",
 "MDNS",
 "MIDI",
 "NS",
 "OS",
+"P2P",
 "PDF",
 "PIN",
 "PNG",
@@ -102,12 +108,14 @@
 "SSO",
 "TCP",
 "TIFF",
+"TOS",
 "TTS",
 "UI",
 "URI",
 "URL",
 "UUID",
 "VC",
+"VO",
 "VOIP",
 "VPN",
 "VR",


Index: clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -42,12 +42,15 @@
 "[2-9]G",
 "ACL",
 "API",
+"APN",
+"APNS",
 "AR",
 "ARGB",
 "ASCII",
 "AV",
 "BGRA",
 "CA",
+"CDN",
 "CF",
 "CG",
 "CI",
@@ -71,14 +74,17 @@
 "ID",
 "JPG",
 "JS",
+"JSON",
 "LAN",
 "LZW",
+"LTR",
 "MAC",
 "MD",
 "MDNS",
 "MIDI",
 "NS",
 "OS",
+"P2P",
 "PDF",
 "PIN",
 "PNG",
@@ -102,12 +108,14 @@
 "SSO",
 "TCP",
 "TIFF",
+"TOS",
 "TTS",
 "UI",
 "URI",
 "URL",
 "UUID",
 "VC",
+"VO",
 "VOIP",
 "VPN",
 "VR",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51832: [clang-tidy/checks] Update objc-property-declaration check to allow arbitrary acronyms and initialisms 🔧

[Ben Hamilton via Phabricator via cfe-commits]

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

This is fine, but please update the comments (and docs?) to make it clear that 
we no longer enforce camelCase but allow aRBiTraRYcAsE now.




Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:28-31
 // For StandardProperty the naming style is 'lowerCamelCase'.
 // For CategoryProperty especially in categories of system class,
 // to avoid naming conflict, the suggested naming style is
 // 'abc_lowerCamelCase' (adding lowercase prefix followed by '_').

These comments are no longer accurate.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:73-74
+  //
+  // Disallow names of this form:
+  // LongString
   std::string StartMatcher = UsedInMatcher ? "::" : "^";

Just to be clear, this also allows things like aRbITRaRyCapS, right? We should 
comment that this is explicitly by design.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:113-115
   // the property name should be in Lower Camel Case like
   // 'lowerCamelCase'
+  unless(matchesName(validPropertyNameRegex(true

This comment is no longer accurate.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51832



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

[PATCH] D42704: [clang-format] Do not break Objective-C string literals inside array literals

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Ping ping!


Repository:
  rC Clang

https://reviews.llvm.org/D42704



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

[PATCH] D42704: [clang-format] Do not break Objective-C string literals inside array literals

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

@stephanemoore: It appears all warnings are enabled by default unless they are 
in class `DefaultIgnore`:

https://github.com/llvm-mirror/clang/blob/6de2efd1953adaa9a190b2cdfbe7b5c15f6d6efe/include/clang/Basic/Diagnostic.td#L105

This diagnostic is not in `DefaultIgnore`:

https://github.com/llvm-mirror/clang/blob/a58d0437d1e17d53d7bffea598d77789dd6d28b6/include/clang/Basic/DiagnosticGroups.td#L933

In fact, the `All` class is specifically for warnings which *are* in 
`DefaultIgnore` but which should be emitted with `-Wall`:

https://github.com/llvm-mirror/clang/blob/a58d0437d1e17d53d7bffea598d77789dd6d28b6/include/clang/Basic/DiagnosticGroups.td#L782

  // Note that putting warnings in -Wall will not disable them by default. If a
  // warning should be active _only_ when -Wall is passed in, mark it as
  // DefaultIgnore in addition to putting it here.


Repository:
  rC Clang

https://reviews.llvm.org/D42704



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

[PATCH] D42650: [clang-format] New format param ObjCBinPackProtocolList

[Ben Hamilton via Phabricator via cfe-commits]

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

> I don't understand why do we introduce an enum option if we are keeping the 
> default behavior for Google style. IMO we should have a single behavior for 
> any style and enforce it.

https://reviews.llvm.org/D42708 changes the default behavior for the Google 
style.

I'm fine with opening up a discussion to make this the default for all styles, 
but I think that should be a separate discussion.


Repository:
  rC Clang

https://reviews.llvm.org/D42650



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

[PATCH] D42947: Support special acronyms inside property names and allow plural forms

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton requested changes to this revision.
benhamilton added a comment.
This revision now requires changes to proceed.

Can you add test cases for non-plural acronyms in the middle of the string and 
plural acronyms at the start/end of the string, please?




Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:142
 
-  return StartMatcher + "((" +
- llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") +
- ")[A-Z]?)?[a-z]+[a-z0-9]*([A-Z][a-z0-9]+)*" + "(" +
- llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") +
- ")?$";
+  return StartMatcher + "(" + AcronymsGroupRegex(EscapedAcronyms, false) +
+ "[A-Z]?)?[a-z]+[a-z0-9]*(" +

Why do we not allow plural acronyms at the start of the property name? For 
example:

```lang=objc
@property(nonatomic) NSArray *URLsToFetch;
```

should be allowed.




Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:144
+ "[A-Z]?)?[a-z]+[a-z0-9]*(" +
+ AcronymsGroupRegex(EscapedAcronyms, true) + "|([A-Z][a-z0-9]+))*$";
 }

Why do we not allow singular acronyms in the middle of the property name?

I think we should allow singular and plural acronyms anywhere.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42947



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

[PATCH] D42864: [clang-format] Add more tests for Objective-C 2.0 generic alignment

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL324364: [clang-format] Add more tests for Objective-C 2.0 
generic alignment (authored by benhamilton, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D42864

Files:
  cfe/trunk/lib/Format/ContinuationIndenter.cpp
  cfe/trunk/unittests/Format/FormatTestObjC.cpp


Index: cfe/trunk/lib/Format/ContinuationIndenter.cpp
===
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp
@@ -1211,7 +1211,6 @@
 // void SomeFunction(vector<  // break
 //   int> v);
 // FIXME: We likely want to do this for more combinations of brackets.
-// Verify that it is wanted for ObjC, too.
 if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {
   NewIndent = std::max(NewIndent, State.Stack.back().Indent);
   LastSpace = std::max(LastSpace, State.Stack.back().Indent);
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -189,6 +189,17 @@
"}\n");
 }
 
+TEST_F(FormatTestObjC, FormatObjCGenerics) {
+  Style.ColumnLimit = 40;
+  verifyFormat("int (\n"
+   "NSArray\n"
+   "a);\n");
+  verifyFormat("int (\n"
+   "NSArray *>\n"
+   "a);\n");
+}
+
 TEST_F(FormatTestObjC, FormatObjCInterface) {
   verifyFormat("@interface Foo : NSObject  {\n"
"@public\n"


Index: cfe/trunk/lib/Format/ContinuationIndenter.cpp
===
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp
@@ -1211,7 +1211,6 @@
 // void SomeFunction(vector<  // break
 //   int> v);
 // FIXME: We likely want to do this for more combinations of brackets.
-// Verify that it is wanted for ObjC, too.
 if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {
   NewIndent = std::max(NewIndent, State.Stack.back().Indent);
   LastSpace = std::max(LastSpace, State.Stack.back().Indent);
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -189,6 +189,17 @@
"}\n");
 }
 
+TEST_F(FormatTestObjC, FormatObjCGenerics) {
+  Style.ColumnLimit = 40;
+  verifyFormat("int (\n"
+   "NSArray\n"
+   "a);\n");
+  verifyFormat("int (\n"
+   "NSArray *>\n"
+   "a);\n");
+}
+
 TEST_F(FormatTestObjC, FormatObjCInterface) {
   verifyFormat("@interface Foo : NSObject  {\n"
"@public\n"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42864: [clang-format] Add more tests for Objective-C 2.0 generic alignment

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC324364: [clang-format] Add more tests for Objective-C 2.0 
generic alignment (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D42864?vs=132654&id=133036#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D42864

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


Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1211,7 +1211,6 @@
 // void SomeFunction(vector<  // break
 //   int> v);
 // FIXME: We likely want to do this for more combinations of brackets.
-// Verify that it is wanted for ObjC, too.
 if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {
   NewIndent = std::max(NewIndent, State.Stack.back().Indent);
   LastSpace = std::max(LastSpace, State.Stack.back().Indent);
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -189,6 +189,17 @@
"}\n");
 }
 
+TEST_F(FormatTestObjC, FormatObjCGenerics) {
+  Style.ColumnLimit = 40;
+  verifyFormat("int (\n"
+   "NSArray\n"
+   "a);\n");
+  verifyFormat("int (\n"
+   "NSArray *>\n"
+   "a);\n");
+}
+
 TEST_F(FormatTestObjC, FormatObjCInterface) {
   verifyFormat("@interface Foo : NSObject  {\n"
"@public\n"


Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1211,7 +1211,6 @@
 // void SomeFunction(vector<  // break
 //   int> v);
 // FIXME: We likely want to do this for more combinations of brackets.
-// Verify that it is wanted for ObjC, too.
 if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {
   NewIndent = std::max(NewIndent, State.Stack.back().Indent);
   LastSpace = std::max(LastSpace, State.Stack.back().Indent);
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -189,6 +189,17 @@
"}\n");
 }
 
+TEST_F(FormatTestObjC, FormatObjCGenerics) {
+  Style.ColumnLimit = 40;
+  verifyFormat("int (\n"
+   "NSArray\n"
+   "a);\n");
+  verifyFormat("int (\n"
+   "NSArray *>\n"
+   "a);\n");
+}
+
 TEST_F(FormatTestObjC, FormatObjCInterface) {
   verifyFormat("@interface Foo : NSObject  {\n"
"@public\n"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42947: Support special acronyms inside property names and allow plural forms

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton accepted this revision.
benhamilton added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:144
+ "[A-Z]?)?[a-z]+[a-z0-9]*(" +
+ AcronymsGroupRegex(EscapedAcronyms, true) + "|([A-Z][a-z0-9]+))*$";
 }

Wizard wrote:
> benhamilton wrote:
> > Why do we not allow singular acronyms in the middle of the property name?
> > 
> > I think we should allow singular and plural acronyms anywhere.
> Actually we do. AcronymsGroupRegex(EscapedAcronyms, true) will support both 
> while AcronymsGroupRegex(EscapedAcronyms, false) only supports singular. Will 
> update test cases.
Ah, this is why I find boolean flags to be confusing :) Anyway, it's gone now.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:139
 
-  return StartMatcher + "((" +
- llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") +
- ")[A-Z]?)?[a-z]+[a-z0-9]*([A-Z][a-z0-9]+)*" + "(" +
- llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") +
- ")?$";
+  return StartMatcher + "(" + AcronymsGroupRegex(EscapedAcronyms) +
+ "[A-Z]?)?[a-z]+[a-z0-9]*(" + AcronymsGroupRegex(EscapedAcronyms) +

Since `AcronymsGroupRegex()` is called twice with the same parameter, please 
store the result in a local variable instead of doing the work twice.



Comment at: test/clang-tidy/objc-property-declaration.m:12
 @property(strong, nonatomic) NSString *URLString;
 @property(strong, nonatomic) NSString *bundleID;
 @property(strong, nonatomic) NSData *RGBABytes;

Add a check for a plural at the end, please.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42947



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

[PATCH] D42708: [clang-format] Set ObjCBinPackProtocolList to Never for google style

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC324553: [clang-format] Set ObjCBinPackProtocolList to Never 
for google style (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D42708?vs=132189&id=133349#toc

Repository:
  rC Clang

https://reviews.llvm.org/D42708

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


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -710,6 +710,7 @@
   GoogleStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
+  GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
   GoogleStyle.ObjCSpaceAfterProperty = false;
   GoogleStyle.ObjCSpaceBeforeProtocolList = true;
   GoogleStyle.PointerAlignment = FormatStyle::PAS_Left;
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -333,13 +333,16 @@
   verifyFormat("@interface Foo (HackStuff) \n"
"+ (id)init;\n"
"@end");
-  Style.BinPackParameters = false;
-  Style.ColumnLimit = 80;
-  verifyFormat("@interface a () <\n"
-   "a,\n"
-   ",\n"
-   "aa,\n"
-   "> {\n"
+  Style.ColumnLimit = 40;
+  // BinPackParameters should be true by default.
+  verifyFormat("void (int e, int e,\n"
+   "  int e, int e);\n");
+  // ObjCBinPackProtocolList should be BPS_Never by default.
+  verifyFormat("@interface f () <\n"
+   "f,\n"
+   "f,\n"
+   "f,\n"
+   "f> {\n"
"}");
 }
 


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -710,6 +710,7 @@
   GoogleStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
+  GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
   GoogleStyle.ObjCSpaceAfterProperty = false;
   GoogleStyle.ObjCSpaceBeforeProtocolList = true;
   GoogleStyle.PointerAlignment = FormatStyle::PAS_Left;
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -333,13 +333,16 @@
   verifyFormat("@interface Foo (HackStuff) \n"
"+ (id)init;\n"
"@end");
-  Style.BinPackParameters = false;
-  Style.ColumnLimit = 80;
-  verifyFormat("@interface a () <\n"
-   "a,\n"
-   ",\n"
-   "aa,\n"
-   "> {\n"
+  Style.ColumnLimit = 40;
+  // BinPackParameters should be true by default.
+  verifyFormat("void (int e, int e,\n"
+   "  int e, int e);\n");
+  // ObjCBinPackProtocolList should be BPS_Never by default.
+  verifyFormat("@interface f () <\n"
+   "f,\n"
+   "f,\n"
+   "f,\n"
+   "f> {\n"
"}");
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42704: [clang-format] Do not break Objective-C string literals inside array literals

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC324618: [clang-format] Do not break Objective-C string 
literals inside array literals (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D42704?vs=132031&id=133428#toc

Repository:
  rC Clang

https://reviews.llvm.org/D42704

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  unittests/Format/FormatTestObjC.cpp


Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1285,6 +1285,9 @@
   State.Stack.back().NestedBlockIndent = NestedBlockIndent;
   State.Stack.back().BreakBeforeParameter = BreakBeforeParameter;
   State.Stack.back().HasMultipleNestedBlocks = Current.BlockParameterCount > 1;
+  State.Stack.back().IsInsideObjCArrayLiteral =
+  Current.is(TT_ArrayInitializerLSquare) && Current.Previous &&
+  Current.Previous->is(tok::at);
 }
 
 void ContinuationIndenter::moveStatePastScopeCloser(LineState &State) {
@@ -1578,6 +1581,11 @@
 // likely want to terminate the string before any line breaking is done.
 if (Current.IsUnterminatedLiteral)
   return nullptr;
+// Don't break string literals inside Objective-C array literals (doing so
+// raises the warning -Wobjc-string-concatenation).
+if (State.Stack.back().IsInsideObjCArrayLiteral) {
+  return nullptr;
+}
 
 StringRef Text = Current.TokenText;
 StringRef Prefix;
Index: lib/Format/ContinuationIndenter.h
===
--- lib/Format/ContinuationIndenter.h
+++ lib/Format/ContinuationIndenter.h
@@ -208,7 +208,8 @@
 NoLineBreakInOperand(false), LastOperatorWrapped(true),
 ContainsLineBreak(false), ContainsUnwrappedBuilder(false),
 AlignColons(true), ObjCSelectorNameFound(false),
-HasMultipleNestedBlocks(false), NestedBlockInlined(false) {}
+HasMultipleNestedBlocks(false), NestedBlockInlined(false),
+IsInsideObjCArrayLiteral(false) {}
 
   /// \brief The position to which a specific parenthesis level needs to be
   /// indented.
@@ -318,6 +319,10 @@
   /// "function" in JavaScript) is not wrapped to a new line.
   bool NestedBlockInlined : 1;
 
+  /// \brief \c true if the current \c ParenState represents an Objective-C
+  /// array literal.
+  bool IsInsideObjCArrayLiteral : 1;
+
   bool operator<(const ParenState &Other) const {
 if (Indent != Other.Indent)
   return Indent < Other.Indent;
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -975,6 +975,12 @@
   verifyFormat("[someFunction someLongParameter:@[\n"
"  NSBundle.mainBundle.infoDictionary[@\"a\"]\n"
"]];");
+  Style.ColumnLimit = 20;
+  // We can't break string literals inside NSArray literals
+  // (that raises -Wobjc-string-concatenation).
+  verifyFormat("NSArray *foo = @[\n"
+   "  @\"aa\"\n"
+   "];\n");
 }
 } // end namespace
 } // end namespace format


Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1285,6 +1285,9 @@
   State.Stack.back().NestedBlockIndent = NestedBlockIndent;
   State.Stack.back().BreakBeforeParameter = BreakBeforeParameter;
   State.Stack.back().HasMultipleNestedBlocks = Current.BlockParameterCount > 1;
+  State.Stack.back().IsInsideObjCArrayLiteral =
+  Current.is(TT_ArrayInitializerLSquare) && Current.Previous &&
+  Current.Previous->is(tok::at);
 }
 
 void ContinuationIndenter::moveStatePastScopeCloser(LineState &State) {
@@ -1578,6 +1581,11 @@
 // likely want to terminate the string before any line breaking is done.
 if (Current.IsUnterminatedLiteral)
   return nullptr;
+// Don't break string literals inside Objective-C array literals (doing so
+// raises the warning -Wobjc-string-concatenation).
+if (State.Stack.back().IsInsideObjCArrayLiteral) {
+  return nullptr;
+}
 
 StringRef Text = Current.TokenText;
 StringRef Prefix;
Index: lib/Format/ContinuationIndenter.h
===
--- lib/Format/ContinuationIndenter.h
+++ lib/Format/ContinuationIndenter.h
@@ -208,7 +208,8 @@
 NoLineBreakInOperand(false), LastOperatorWrapped(true),
 ContainsLineBreak(false), ContainsUnwrappedBuilder(false),
 AlignColons(true), ObjCSelectorNameFound(false),
-HasMultipleNestedBlocks(false), NestedBlockInlined(false) {}
+HasMultipleNestedBlocks(false), NestedBlockInlined(false),
+IsInsideObjCArrayLiteral(fa

[PATCH] D43124: [clang-format] Improve ObjC headers detection

[Ben Hamilton via Phabricator via cfe-commits]

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

testcase 



Repository:
  rC Clang

https://reviews.llvm.org/D43124



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

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();

Typz wrote:
> Wondering if formatting with this style is appropriate: the clang-format doc 
> points in that direction, but it seems to me both `@synchronized` and 
> `@autoreleasepool` are more akin to "control statements".
> 
> For consistency (esp. in ObjC++ code), as a user, I would tend to have these 
> blocks indented like control statements while interfaces/implementations 
> blocks would be indented like classes/structs.
> 
> So maybe it would be better to introduce a new BraceWrapping style 
> 'AfterObjCSpecialBlock` to control these cases, matching the possibilities 
> that are given for control statements vs classes. What do you think?
Hmm, I definitely agree with that logic. I don't see them acting as 
declarations in any way, they are definitely like control statements.



Repository:
  rC Clang

https://reviews.llvm.org/D43114



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

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+  if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();

Typz wrote:
> benhamilton wrote:
> > Typz wrote:
> > > Wondering if formatting with this style is appropriate: the clang-format 
> > > doc points in that direction, but it seems to me both `@synchronized` and 
> > > `@autoreleasepool` are more akin to "control statements".
> > > 
> > > For consistency (esp. in ObjC++ code), as a user, I would tend to have 
> > > these blocks indented like control statements while 
> > > interfaces/implementations blocks would be indented like classes/structs.
> > > 
> > > So maybe it would be better to introduce a new BraceWrapping style 
> > > 'AfterObjCSpecialBlock` to control these cases, matching the 
> > > possibilities that are given for control statements vs classes. What do 
> > > you think?
> > Hmm, I definitely agree with that logic. I don't see them acting as 
> > declarations in any way, they are definitely like control statements.
> > 
> Ok, i can change this. Two questions though:
> * Should I do this in this patch, or a separate patch? (won't be a big change 
> in any case, but it can still be seen as 2 separate issues/changes)
> * Should I introduce a new BraceWrapping flag (`AfterObjCSpecialBlock`), or 
> simply apply `AfterControlStatement` to these blocks?
Personally, I feel `AfterControlStatement` applies to these. I'm not an expert 
on this, though, so I will defer to others on the review.


Repository:
  rC Clang

https://reviews.llvm.org/D43114



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton requested changes to this revision.
benhamilton added a comment.
This revision now requires changes to proceed.

Thanks! Can you add a test for this, please?


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

> This used to happen before as well, and would require fixing 
> UnwrappedLineFormatter.cpp to understand that these blocks are linked to the 
> previous ObjC keyword.

Good find! Looks like we should fix that and add some more tests with 
AfterControlStatement set to true and to false.


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks

[Ben Hamilton via Phabricator via cfe-commits]

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

Just a question on the test.




Comment at: unittests/Format/FormatTestObjC.cpp:193-198
+  verifyFormat("@synchronized(self) {\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self) {\n"
+   "  f();\n"
+   "}\n");

Why is the block repeated twice?



Comment at: unittests/Format/FormatTestObjC.cpp:200-207
+  verifyFormat("@synchronized(self)\n"
+   "{\n"
+   "  f();\n"
+   "}\n"
+   "@synchronized(self)\n"
+   "{\n"
+   "  f();\n"

Same question.


Repository:
  rC Clang

https://reviews.llvm.org/D43114



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

[PATCH] D43522: [clang-format] New API guessLanguage()

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added a reviewer: jolesiak.
Herald added subscribers: cfe-commits, klimek.
benhamilton added a reviewer: krasimir.

For clients which don't have a filesystem, calling getStyle() doesn't
make much sense (there's no .clang-format files to search for).

In this diff, I hoist out the language-guessing logic from getStyle()
and move it into a new API guessLanguage().

I also added support for guessing the language of files which have no
extension (they could be C++ or ObjC).

Test Plan: New tests added. Ran tests with:

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


Repository:
  rC Clang

https://reviews.llvm.org/D43522

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11952,6 +11952,34 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
+struct GuessLanguageTestCase {
+  const char *const FileName;
+  const char *const Code;
+  const FormatStyle::LanguageKind ExpectedResult;
+};
+
+class GuessLanguageTest
+: public FormatTest,
+  public ::testing::WithParamInterface {};
+
+TEST_P(GuessLanguageTest, FileAndCode) {
+  auto TestCase = GetParam();
+  EXPECT_EQ(TestCase.ExpectedResult,
+guessLanguage(TestCase.FileName, TestCase.Code));
+}
+
+static const GuessLanguageTestCase TestCases[] = {
+{"foo.cc", "", FormatStyle::LK_Cpp},
+{"foo.m", "", FormatStyle::LK_ObjC},
+{"foo.mm", "", FormatStyle::LK_ObjC},
+{"foo.h", "", FormatStyle::LK_Cpp},
+{"foo.h", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
+{"foo", "", FormatStyle::LK_Cpp},
+{"foo", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
+};
+INSTANTIATE_TEST_CASE_P(ValidLanguages, GuessLanguageTest,
+::testing::ValuesIn(TestCases));
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2294,24 +2294,33 @@
   return FormatStyle::LK_Cpp;
 }
 
+FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
+  FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
+  if (result == FormatStyle::LK_Cpp) {
+auto extension = llvm::sys::path::extension(FileName);
+// If there's no file extension (or it's .h), we need to check the contents
+// of the code to see if it contains Objective-C.
+if (extension.empty() || extension == ".h") {
+  std::unique_ptr Env =
+  Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
+  ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
+  Guesser.process();
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+  }
+}
+  }
+  return result;
+}
+
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
  StringRef FallbackStyleName,
  StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
 FS = vfs::getRealFileSystem().get();
   }
   FormatStyle Style = getLLVMStyle();
-  Style.Language = getLanguageByFileName(FileName);
-
-  if (Style.Language == FormatStyle::LK_Cpp && FileName.endswith(".h")) {
-std::unique_ptr Env =
-Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
-ObjCHeaderStyleGuesser Guesser(*Env, Style);
-Guesser.process();
-if (Guesser.isObjC()) {
-  Style.Language = FormatStyle::LK_ObjC;
-}
-  }
+  Style.Language = guessLanguage(FileName, Code);
 
   FormatStyle FallbackStyle = getNoStyle();
   if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -1981,6 +1981,10 @@
  StringRef Code = "",
  vfs::FileSystem *FS = nullptr);
 
+// \brief Guesses the language from the ``FileName`` and ``Code`` to be formatted.
+// Defaults to FormatStyle::LK_Cpp.
+FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code);
+
 // \brief Returns a string representation of ``Language``.
 inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {
   switch (Language) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43522: [clang-format] New API guessLanguage()

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC325691: [clang-format] New API guessLanguage() (authored by 
benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43522?vs=135121&id=135261#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43522

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2294,24 +2294,33 @@
   return FormatStyle::LK_Cpp;
 }
 
+FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
+  FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
+  if (result == FormatStyle::LK_Cpp) {
+auto extension = llvm::sys::path::extension(FileName);
+// If there's no file extension (or it's .h), we need to check the contents
+// of the code to see if it contains Objective-C.
+if (extension.empty() || extension == ".h") {
+  std::unique_ptr Env =
+  Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
+  ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
+  Guesser.process();
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+  }
+}
+  }
+  return result;
+}
+
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
  StringRef FallbackStyleName,
  StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
 FS = vfs::getRealFileSystem().get();
   }
   FormatStyle Style = getLLVMStyle();
-  Style.Language = getLanguageByFileName(FileName);
-
-  if (Style.Language == FormatStyle::LK_Cpp && FileName.endswith(".h")) {
-std::unique_ptr Env =
-Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
-ObjCHeaderStyleGuesser Guesser(*Env, Style);
-Guesser.process();
-if (Guesser.isObjC()) {
-  Style.Language = FormatStyle::LK_ObjC;
-}
-  }
+  Style.Language = guessLanguage(FileName, Code);
 
   FormatStyle FallbackStyle = getNoStyle();
   if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11952,6 +11952,34 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
+struct GuessLanguageTestCase {
+  const char *const FileName;
+  const char *const Code;
+  const FormatStyle::LanguageKind ExpectedResult;
+};
+
+class GuessLanguageTest
+: public FormatTest,
+  public ::testing::WithParamInterface {};
+
+TEST_P(GuessLanguageTest, FileAndCode) {
+  auto TestCase = GetParam();
+  EXPECT_EQ(TestCase.ExpectedResult,
+guessLanguage(TestCase.FileName, TestCase.Code));
+}
+
+static const GuessLanguageTestCase TestCases[] = {
+{"foo.cc", "", FormatStyle::LK_Cpp},
+{"foo.m", "", FormatStyle::LK_ObjC},
+{"foo.mm", "", FormatStyle::LK_ObjC},
+{"foo.h", "", FormatStyle::LK_Cpp},
+{"foo.h", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
+{"foo", "", FormatStyle::LK_Cpp},
+{"foo", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
+};
+INSTANTIATE_TEST_CASE_P(ValidLanguages, GuessLanguageTest,
+::testing::ValuesIn(TestCases));
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -1981,6 +1981,10 @@
  StringRef Code = "",
  vfs::FileSystem *FS = nullptr);
 
+// \brief Guesses the language from the ``FileName`` and ``Code`` to be formatted.
+// Defaults to FormatStyle::LK_Cpp.
+FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code);
+
 // \brief Returns a string representation of ``Language``.
 inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {
   switch (Language) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43522: [clang-format] New API guessLanguage()

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325691: [clang-format] New API guessLanguage() (authored by 
benhamilton, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D43522

Files:
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -1981,6 +1981,10 @@
  StringRef Code = "",
  vfs::FileSystem *FS = nullptr);
 
+// \brief Guesses the language from the ``FileName`` and ``Code`` to be formatted.
+// Defaults to FormatStyle::LK_Cpp.
+FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code);
+
 // \brief Returns a string representation of ``Language``.
 inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {
   switch (Language) {
Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -2294,24 +2294,33 @@
   return FormatStyle::LK_Cpp;
 }
 
+FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
+  FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
+  if (result == FormatStyle::LK_Cpp) {
+auto extension = llvm::sys::path::extension(FileName);
+// If there's no file extension (or it's .h), we need to check the contents
+// of the code to see if it contains Objective-C.
+if (extension.empty() || extension == ".h") {
+  std::unique_ptr Env =
+  Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
+  ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
+  Guesser.process();
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+  }
+}
+  }
+  return result;
+}
+
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
  StringRef FallbackStyleName,
  StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
 FS = vfs::getRealFileSystem().get();
   }
   FormatStyle Style = getLLVMStyle();
-  Style.Language = getLanguageByFileName(FileName);
-
-  if (Style.Language == FormatStyle::LK_Cpp && FileName.endswith(".h")) {
-std::unique_ptr Env =
-Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
-ObjCHeaderStyleGuesser Guesser(*Env, Style);
-Guesser.process();
-if (Guesser.isObjC()) {
-  Style.Language = FormatStyle::LK_ObjC;
-}
-  }
+  Style.Language = guessLanguage(FileName, Code);
 
   FormatStyle FallbackStyle = getNoStyle();
   if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -11952,6 +11952,34 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
+struct GuessLanguageTestCase {
+  const char *const FileName;
+  const char *const Code;
+  const FormatStyle::LanguageKind ExpectedResult;
+};
+
+class GuessLanguageTest
+: public FormatTest,
+  public ::testing::WithParamInterface {};
+
+TEST_P(GuessLanguageTest, FileAndCode) {
+  auto TestCase = GetParam();
+  EXPECT_EQ(TestCase.ExpectedResult,
+guessLanguage(TestCase.FileName, TestCase.Code));
+}
+
+static const GuessLanguageTestCase TestCases[] = {
+{"foo.cc", "", FormatStyle::LK_Cpp},
+{"foo.m", "", FormatStyle::LK_ObjC},
+{"foo.mm", "", FormatStyle::LK_ObjC},
+{"foo.h", "", FormatStyle::LK_Cpp},
+{"foo.h", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
+{"foo", "", FormatStyle::LK_Cpp},
+{"foo", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
+};
+INSTANTIATE_TEST_CASE_P(ValidLanguages, GuessLanguageTest,
+::testing::ValuesIn(TestCases));
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43522: [clang-format] New API guessLanguage()

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: cfe/trunk/lib/Format/Format.cpp:2298
+FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
+  FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
+  if (result == FormatStyle::LK_Cpp) {

djasper wrote:
> Here and in several other places: Variables should be named with upper camel 
> case 
> (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).
Thanks, will send a follow-up.



Comment at: cfe/trunk/lib/Format/Format.cpp:2308
+  Guesser.process();
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;

djasper wrote:
> In LLVM, we generally don't add braces for single statement ifs.
Mmmm.. is this a hard requirement? I've personally been bitten so many times by 
adding statements after missing braces, I'd rather add them unless they're 
required to not be there by the style guide.



Comment at: cfe/trunk/lib/Format/Format.cpp:2309
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+  }

djasper wrote:
> Why not just return here?
I don't like early returns in case an else sneaks in later:

https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return



Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955
 
+struct GuessLanguageTestCase {
+  const char *const FileName;

djasper wrote:
> IMO, this is a bit over-engineered. IIUC, this only calls a single free 
> standing function with two different parameters and expects a certain result. 
> You don't need this struct, a test fixture or parameterized tests for that. 
> Just write:
> 
>   TEST(GuessLanguageTest, FileAndCode) {
> EXPECT_EQ(guessLanguage("foo.cc", ""), FormatStyle::LK_Cpp);
> EXPECT_EQ(guessLanguage("foo.m", ""), FormatStyle::LK_ObjC);
> ...
>   }
> 
> Yes, you'd be duplicating the "EXPECT_EQ" and "guessLanguage", but I think 
> that's actually good here. It makes the tests intuitively readable.
I hear you. I much prefer it when a single test tests a single issue, so 
failures are isolated to the actual change:

https://elgaard.blog/2011/02/06/multiple-asserts-in-a-single-unit-test-method/

If this isn't a hard requirement, I'd like to keep this the way it is.


Repository:
  rL LLVM

https://reviews.llvm.org/D43522



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

[PATCH] D43590: [clang-format] Fix regression when getStyle() called with empty filename

[Ben Hamilton via Phabricator via cfe-commits]

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

https://reviews.llvm.org/D43522 caused an assertion failure when getStyle() was 
called with
an empty filename:

https://reviews.llvm.org/P8065

This adds a test to reproduce the failure and fixes the issue by
ensuring we never pass an empty filename to
Environment::CreateVirtualEnvironment().

Test Plan: New test added. Ran test with:

  % make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests
  Before diff, test failed with P8065. Now, test passes.


Repository:
  rC Clang

https://reviews.llvm.org/D43590

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11723,6 +11723,12 @@
 verifyFormat("__super::FooBar();");
 }
 
+TEST(FormatStyle, GetStyleWithEmptyFileName) {
+  auto Style1 = getStyle("file", "", "Google");
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getGoogleStyle());
+}
+
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2297,12 +2297,13 @@
 FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
   FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
   if (result == FormatStyle::LK_Cpp) {
-auto extension = llvm::sys::path::extension(FileName);
+auto Extension = llvm::sys::path::extension(FileName);
 // If there's no file extension (or it's .h), we need to check the contents
 // of the code to see if it contains Objective-C.
-if (extension.empty() || extension == ".h") {
+if (Extension.empty() || Extension == ".h") {
+  auto NonEmptyFileName = FileName.empty() ? "guess.h" : FileName;
   std::unique_ptr Env =
-  Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
+  Environment::CreateVirtualEnvironment(Code, NonEmptyFileName, 
/*Ranges=*/{});
   ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
   Guesser.process();
   if (Guesser.isObjC()) {


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11723,6 +11723,12 @@
 verifyFormat("__super::FooBar();");
 }
 
+TEST(FormatStyle, GetStyleWithEmptyFileName) {
+  auto Style1 = getStyle("file", "", "Google");
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getGoogleStyle());
+}
+
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2297,12 +2297,13 @@
 FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
   FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
   if (result == FormatStyle::LK_Cpp) {
-auto extension = llvm::sys::path::extension(FileName);
+auto Extension = llvm::sys::path::extension(FileName);
 // If there's no file extension (or it's .h), we need to check the contents
 // of the code to see if it contains Objective-C.
-if (extension.empty() || extension == ".h") {
+if (Extension.empty() || Extension == ".h") {
+  auto NonEmptyFileName = FileName.empty() ? "guess.h" : FileName;
   std::unique_ptr Env =
-  Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
+  Environment::CreateVirtualEnvironment(Code, NonEmptyFileName, /*Ranges=*/{});
   ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
   Guesser.process();
   if (Guesser.isObjC()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43590: [clang-format] Fix regression when getStyle() called with empty filename

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC325722: [clang-format] Fix regression when getStyle() called 
with empty filename (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43590?vs=135314&id=135321#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43590

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


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2297,12 +2297,13 @@
 FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
   FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
   if (result == FormatStyle::LK_Cpp) {
-auto extension = llvm::sys::path::extension(FileName);
+auto Extension = llvm::sys::path::extension(FileName);
 // If there's no file extension (or it's .h), we need to check the contents
 // of the code to see if it contains Objective-C.
-if (extension.empty() || extension == ".h") {
+if (Extension.empty() || Extension == ".h") {
+  auto NonEmptyFileName = FileName.empty() ? "guess.h" : FileName;
   std::unique_ptr Env =
-  Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
+  Environment::CreateVirtualEnvironment(Code, NonEmptyFileName, 
/*Ranges=*/{});
   ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
   Guesser.process();
   if (Guesser.isObjC()) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11723,6 +11723,12 @@
 verifyFormat("__super::FooBar();");
 }
 
+TEST(FormatStyle, GetStyleWithEmptyFileName) {
+  auto Style1 = getStyle("file", "", "Google");
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getGoogleStyle());
+}
+
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2297,12 +2297,13 @@
 FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
   FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
   if (result == FormatStyle::LK_Cpp) {
-auto extension = llvm::sys::path::extension(FileName);
+auto Extension = llvm::sys::path::extension(FileName);
 // If there's no file extension (or it's .h), we need to check the contents
 // of the code to see if it contains Objective-C.
-if (extension.empty() || extension == ".h") {
+if (Extension.empty() || Extension == ".h") {
+  auto NonEmptyFileName = FileName.empty() ? "guess.h" : FileName;
   std::unique_ptr Env =
-  Environment::CreateVirtualEnvironment(Code, FileName, /*Ranges=*/{});
+  Environment::CreateVirtualEnvironment(Code, NonEmptyFileName, /*Ranges=*/{});
   ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
   Guesser.process();
   if (Guesser.isObjC()) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11723,6 +11723,12 @@
 verifyFormat("__super::FooBar();");
 }
 
+TEST(FormatStyle, GetStyleWithEmptyFileName) {
+  auto Style1 = getStyle("file", "", "Google");
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getGoogleStyle());
+}
+
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43522: [clang-format] New API guessLanguage()

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: cfe/trunk/lib/Format/Format.cpp:2308
+  Guesser.process();
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;

djasper wrote:
> benhamilton wrote:
> > djasper wrote:
> > > In LLVM, we generally don't add braces for single statement ifs.
> > Mmmm.. is this a hard requirement? I've personally been bitten so many 
> > times by adding statements after missing braces, I'd rather add them unless 
> > they're required to not be there by the style guide.
> Yes. This is done as consistently as possible throughout LLVM and Clang and 
> I'd like to keep clang-format's codebase consistent.
> 
> clang-format itself makes it really hard to get bitten by missing braces :).
OK, will remove the braces for consistency. Maybe we should make clang-format 
do this automatically?



Comment at: cfe/trunk/lib/Format/Format.cpp:2309
+  if (Guesser.isObjC()) {
+result = FormatStyle::LK_ObjC;
+  }

djasper wrote:
> benhamilton wrote:
> > djasper wrote:
> > > Why not just return here?
> > I don't like early returns in case an else sneaks in later:
> > 
> > https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
> But I would argue exactly the opposite. At this point, you have pretty 
> uniquely determined that this is ObjC (from this originally being viewed as 
> LK_Cpp). Now suppose you add additional logic before the return statement in 
> line 2313: That would make the state space that this function can have quite 
> complex.
> 
> I would even go one step further and completely eliminate the variable 
> "result". That would be slightly less efficient, so maybe I'd be ok with:
> 
>   const auto GuessFromFilename = getLanguageByFileName(FileName);
> 
> And then you can return this at the end or have early exits / other code 
> paths if you come up with different languages. Having both, a complex control 
> flow and state in local variables seems unnecessarily complex here.
OK, happy to change it.



Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955
 
+struct GuessLanguageTestCase {
+  const char *const FileName;

djasper wrote:
> benhamilton wrote:
> > djasper wrote:
> > > IMO, this is a bit over-engineered. IIUC, this only calls a single free 
> > > standing function with two different parameters and expects a certain 
> > > result. You don't need this struct, a test fixture or parameterized tests 
> > > for that. Just write:
> > > 
> > >   TEST(GuessLanguageTest, FileAndCode) {
> > > EXPECT_EQ(guessLanguage("foo.cc", ""), FormatStyle::LK_Cpp);
> > > EXPECT_EQ(guessLanguage("foo.m", ""), FormatStyle::LK_ObjC);
> > > ...
> > >   }
> > > 
> > > Yes, you'd be duplicating the "EXPECT_EQ" and "guessLanguage", but I 
> > > think that's actually good here. It makes the tests intuitively readable.
> > I hear you. I much prefer it when a single test tests a single issue, so 
> > failures are isolated to the actual change:
> > 
> > https://elgaard.blog/2011/02/06/multiple-asserts-in-a-single-unit-test-method/
> > 
> > If this isn't a hard requirement, I'd like to keep this the way it is.
> It certainly makes sense for asserts, as a tests stops upon finding the first 
> assert. But these are expectations. Each failing expectation will be reported 
> individually, with a direct reference to the line in question and an easily 
> understandable error message.
> 
> I understand what you are saying but I think my proposal will actually make 
> test failures easier to diagnose and understand. Please do change it.
Thanks, I appreciate the feedback and will make the change.


Repository:
  rL LLVM

https://reviews.llvm.org/D43522



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

[PATCH] D43522: [clang-format] New API guessLanguage()

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Follow-ups (aside from the case, which I already fixed) in 
https://reviews.llvm.org/D43598.


Repository:
  rL LLVM

https://reviews.llvm.org/D43522



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

[PATCH] D43598: [clang-format] Tidy up new API guessLanguage()

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added a reviewer: djasper.
Herald added subscribers: cfe-commits, klimek.

This fixes a few issues djasper@ brought up in his review of 
https://reviews.llvm.org/D43522.

Test Plan:

  make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests


Repository:
  rC Clang

https://reviews.llvm.org/D43598

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11958,34 +11958,16 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
-struct GuessLanguageTestCase {
-  const char *const FileName;
-  const char *const Code;
-  const FormatStyle::LanguageKind ExpectedResult;
-};
-
-class GuessLanguageTest
-: public FormatTest,
-  public ::testing::WithParamInterface {};
-
-TEST_P(GuessLanguageTest, FileAndCode) {
-  auto TestCase = GetParam();
-  EXPECT_EQ(TestCase.ExpectedResult,
-guessLanguage(TestCase.FileName, TestCase.Code));
+TEST_F(FormatTest, FileAndCode) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.cc", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.m", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.mm", ""));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "@interface 
Foo\n@end\n"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface 
Foo\n@end\n"));
 }
 
-static const GuessLanguageTestCase TestCases[] = {
-{"foo.cc", "", FormatStyle::LK_Cpp},
-{"foo.m", "", FormatStyle::LK_ObjC},
-{"foo.mm", "", FormatStyle::LK_ObjC},
-{"foo.h", "", FormatStyle::LK_Cpp},
-{"foo.h", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
-{"foo", "", FormatStyle::LK_Cpp},
-{"foo", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
-};
-INSTANTIATE_TEST_CASE_P(ValidLanguages, GuessLanguageTest,
-::testing::ValuesIn(TestCases));
-
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2295,8 +2295,8 @@
 }
 
 FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
-  FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
-  if (result == FormatStyle::LK_Cpp) {
+  const auto GuessedLanguage = getLanguageByFileName(FileName);
+  if (GuessedLanguage == FormatStyle::LK_Cpp) {
 auto Extension = llvm::sys::path::extension(FileName);
 // If there's no file extension (or it's .h), we need to check the contents
 // of the code to see if it contains Objective-C.
@@ -2306,12 +2306,11 @@
   Environment::CreateVirtualEnvironment(Code, NonEmptyFileName, 
/*Ranges=*/{});
   ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
   Guesser.process();
-  if (Guesser.isObjC()) {
-result = FormatStyle::LK_ObjC;
-  }
+  if (Guesser.isObjC())
+return FormatStyle::LK_ObjC;
 }
   }
-  return result;
+  return GuessedLanguage;
 }
 
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11958,34 +11958,16 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
-struct GuessLanguageTestCase {
-  const char *const FileName;
-  const char *const Code;
-  const FormatStyle::LanguageKind ExpectedResult;
-};
-
-class GuessLanguageTest
-: public FormatTest,
-  public ::testing::WithParamInterface {};
-
-TEST_P(GuessLanguageTest, FileAndCode) {
-  auto TestCase = GetParam();
-  EXPECT_EQ(TestCase.ExpectedResult,
-guessLanguage(TestCase.FileName, TestCase.Code));
+TEST_F(FormatTest, FileAndCode) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.cc", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.m", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.mm", ""));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "@interface Foo\n@end\n"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n@end\n"));
 }
 
-static const GuessLanguageTestCase TestCases[] = {
-{"foo.cc", "", FormatStyle::LK_Cpp},
-{"foo.m", "", FormatStyle::LK_ObjC},
-{"foo.mm", "", FormatStyle::LK_ObjC},
-{"foo.h", "", FormatStyle::LK_Cpp},
-{"foo.h", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
-{"foo", "", FormatStyle::LK_Cpp},
-{"foo", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
-};
-INSTANTIATE_TES

[PATCH] D41195: [ClangFormat] IndentWrappedFunctionNames should be true in the google ObjC style

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 126969.
benhamilton marked an inline comment as done.
benhamilton added a comment.

- Use 40 column limit for test.


Repository:
  rC Clang

https://reviews.llvm.org/D41195

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -382,9 +382,9 @@
"ofSize:(size_t)height\n"
"  :(size_t)width;");
 
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
   // Continuation indent width should win over aligning colons if the function
   // name is long.
-  Style = getGoogleStyle(FormatStyle::LK_ObjC);
   Style.ColumnLimit = 40;
   Style.IndentWrappedFunctionNames = true;
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
@@ -395,7 +395,10 @@
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
"   aShortf:(NSRect)theRect {\n"
"}");
-
+  // Wrapped method parameters should be indented.
+  verifyFormat("- (LongReturnTypeName)\n"
+   "longParam:(ParamName)longParamName\n"
+   "param:(paramName)paramName;");
   // Format pairs correctly.
   Style.ColumnLimit = 80;
   verifyFormat("- (void)drawRectOn:(id)surface\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -732,6 +732,7 @@
 GoogleStyle.SpacesInContainerLiterals = false;
   } else if (Language == FormatStyle::LK_ObjC) {
 GoogleStyle.ColumnLimit = 100;
+GoogleStyle.IndentWrappedFunctionNames = true;
   }
 
   return GoogleStyle;


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -382,9 +382,9 @@
"ofSize:(size_t)height\n"
"  :(size_t)width;");
 
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
   // Continuation indent width should win over aligning colons if the function
   // name is long.
-  Style = getGoogleStyle(FormatStyle::LK_ObjC);
   Style.ColumnLimit = 40;
   Style.IndentWrappedFunctionNames = true;
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
@@ -395,7 +395,10 @@
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
"   aShortf:(NSRect)theRect {\n"
"}");
-
+  // Wrapped method parameters should be indented.
+  verifyFormat("- (LongReturnTypeName)\n"
+   "longParam:(ParamName)longParamName\n"
+   "param:(paramName)paramName;");
   // Format pairs correctly.
   Style.ColumnLimit = 80;
   verifyFormat("- (void)drawRectOn:(id)surface\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -732,6 +732,7 @@
 GoogleStyle.SpacesInContainerLiterals = false;
   } else if (Language == FormatStyle::LK_ObjC) {
 GoogleStyle.ColumnLimit = 100;
+GoogleStyle.IndentWrappedFunctionNames = true;
   }
 
   return GoogleStyle;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41195: [ClangFormat] IndentWrappedFunctionNames should be true in the google ObjC style

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: unittests/Format/FormatTestObjC.cpp:388
+  // Wrapped method parameters should be indented.
+  verifyFormat("- (VeryLongReturnTypeName)\n"
+   "veryLongMethodParameter:(VeryLongParameterName)"

djasper wrote:
> Set Style.ColumnLimit to something lower so that you don't have to wrap 
> single lines (for better test readability).
Good call, done. I was wondering why tests were doing that, should have 
realized.


Repository:
  rC Clang

https://reviews.llvm.org/D41195



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

[PATCH] D41195: [ClangFormat] IndentWrappedFunctionNames should be true in the google ObjC style

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC320714: [ClangFormat] IndentWrappedFunctionNames should be 
true in the google ObjC style (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41195?vs=126969&id=126970#toc

Repository:
  rC Clang

https://reviews.llvm.org/D41195

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


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -732,6 +732,7 @@
 GoogleStyle.SpacesInContainerLiterals = false;
   } else if (Language == FormatStyle::LK_ObjC) {
 GoogleStyle.ColumnLimit = 100;
+GoogleStyle.IndentWrappedFunctionNames = true;
   }
 
   return GoogleStyle;
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -382,9 +382,9 @@
"ofSize:(size_t)height\n"
"  :(size_t)width;");
 
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
   // Continuation indent width should win over aligning colons if the function
   // name is long.
-  Style = getGoogleStyle(FormatStyle::LK_ObjC);
   Style.ColumnLimit = 40;
   Style.IndentWrappedFunctionNames = true;
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
@@ -395,7 +395,10 @@
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
"   aShortf:(NSRect)theRect {\n"
"}");
-
+  // Wrapped method parameters should be indented.
+  verifyFormat("- (LongReturnTypeName)\n"
+   "longParam:(ParamName)longParamName\n"
+   "param:(paramName)paramName;");
   // Format pairs correctly.
   Style.ColumnLimit = 80;
   verifyFormat("- (void)drawRectOn:(id)surface\n"


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -732,6 +732,7 @@
 GoogleStyle.SpacesInContainerLiterals = false;
   } else if (Language == FormatStyle::LK_ObjC) {
 GoogleStyle.ColumnLimit = 100;
+GoogleStyle.IndentWrappedFunctionNames = true;
   }
 
   return GoogleStyle;
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -382,9 +382,9 @@
"ofSize:(size_t)height\n"
"  :(size_t)width;");
 
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
   // Continuation indent width should win over aligning colons if the function
   // name is long.
-  Style = getGoogleStyle(FormatStyle::LK_ObjC);
   Style.ColumnLimit = 40;
   Style.IndentWrappedFunctionNames = true;
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
@@ -395,7 +395,10 @@
   verifyFormat("- (void)shortf:(GTMFoo *)theFoo\n"
"   aShortf:(NSRect)theRect {\n"
"}");
-
+  // Wrapped method parameters should be indented.
+  verifyFormat("- (LongReturnTypeName)\n"
+   "longParam:(ParamName)longParamName\n"
+   "param:(paramName)paramName;");
   // Format pairs correctly.
   Style.ColumnLimit = 80;
   verifyFormat("- (void)drawRectOn:(id)surface\n"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41789: [clang-tidy] Function-scoped static variables should not trigger google-objc-global-variable-declaration

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: Wizard, hokein, klimek.
Herald added subscribers: cfe-commits, xazax.hun.

google-objc-global-variable-declaration currently triggers on
valid code like:

- (void)foo { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ /* 
... */ }); }

The Google Objective-C style guide says:

http://google.github.io/styleguide/objcguide.html#common-variable-names

> File scope or global variables (as opposed to constants) declared
>  outside the scope of a method or function should be rare, and should
>  have the prefix g.

which is meant to insinuate that static variables inside a method or
function don't need a special name.

Test Plan: `make -j12 check-clang-tools`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41789

Files:
  clang-tidy/google/GlobalVariableDeclarationCheck.cpp
  test/clang-tidy/google-objc-global-variable-declaration.m


Index: test/clang-tidy/google-objc-global-variable-declaration.m
===
--- test/clang-tidy/google-objc-global-variable-declaration.m
+++ test/clang-tidy/google-objc-global-variable-declaration.m
@@ -30,12 +30,12 @@
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
 static NSString* const kGood = @"hello";
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:24: warning: const global variable 'kGood' 
must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
 static NSString* gMyIntGood = 0;
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:18: warning: non-const global variable 
'gMyIntGood' must have a name which starts with 'g[A-Z]' 
[google-objc-global-variable-declaration]
+
 @implementation Foo
 - (void)f {
 int x = 0;
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:9: warning: non-const global variable 
'gMyIntGood' must have a name which starts with 'g[A-Z]' 
[google-objc-global-variable-declaration]
+static int bar;
+static const int baz = 42;
 }
 @end
Index: clang-tidy/google/GlobalVariableDeclarationCheck.cpp
===
--- clang-tidy/google/GlobalVariableDeclarationCheck.cpp
+++ clang-tidy/google/GlobalVariableDeclarationCheck.cpp
@@ -24,6 +24,10 @@
 
 namespace {
 
+AST_MATCHER(VarDecl, isLocalVariable) {
+  return Node.isLocalVarDecl();
+}
+
 FixItHint generateFixItHint(const VarDecl *Decl, bool IsConst) {
   char FC = Decl->getName()[0];
   if (!llvm::isAlpha(FC) || Decl->getName().size() == 1) {
@@ -57,12 +61,17 @@
   // need to add two matchers since we need to bind different ids to 
distinguish
   // constants and variables. Since bind() can only be called on node matchers,
   // we cannot make it in one matcher.
+  //
+  // Note that hasGlobalStorage() matches static variables declared locally
+  // inside a function or method, so we need to exclude those with
+  // isLocalVariable().
   Finder->addMatcher(
   varDecl(hasGlobalStorage(), unless(hasType(isConstQualified())),
-  unless(matchesName("::g[A-Z]")))
+  unless(isLocalVariable()), unless(matchesName("::g[A-Z]")))
   .bind("global_var"),
   this);
   Finder->addMatcher(varDecl(hasGlobalStorage(), hasType(isConstQualified()),
+ unless(isLocalVariable()),
  unless(matchesName("::k[A-Z]")))
  .bind("global_const"),
  this);


Index: test/clang-tidy/google-objc-global-variable-declaration.m
===
--- test/clang-tidy/google-objc-global-variable-declaration.m
+++ test/clang-tidy/google-objc-global-variable-declaration.m
@@ -30,12 +30,12 @@
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
 static NSString* const kGood = @"hello";
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:24: warning: const global variable 'kGood' must have a name which starts with 'k[A-Z]' [google-objc-global-variable-declaration]
 static NSString* gMyIntGood = 0;
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:18: warning: non-const global variable 'gMyIntGood' must have a name which starts with 'g[A-Z]' [google-objc-global-variable-declaration]
+
 @implementation Foo
 - (void)f {
 int x = 0;
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:9: warning: non-const global variable 'gMyIntGood' must have a name which starts with 'g[A-Z]' [google-objc-global-variable-declaration]
+static int bar;
+static const int baz = 42;
 }
 @end
Index: clang-tidy/google/GlobalVariableDeclarationCheck.cpp
===
--- clang-tidy/google/GlobalVariableDeclarationCheck.cpp
+++ clang-tidy/google/GlobalVariableDeclarationCheck.cpp
@@ -24,6 +24,10 @@
 
 namespace {
 
+AST_MATCHER(VarDecl, isLocalVariable) {
+  return Node.isLocalVarDecl();
+}
+
 FixItHint generateFixItHint(const VarDecl *Decl, bool IsConst) {
   char FC = Decl->getName()[0];
   if (!llvm::isAlpha(FC) || Decl->g

[PATCH] D41789: [clang-tidy] Function-scoped static variables should not trigger google-objc-global-variable-declaration

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE321914: [clang-tidy] Function-scoped static variables 
should not trigger google-objc… (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41789?vs=128814&id=128819#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41789

Files:
  clang-tidy/google/GlobalVariableDeclarationCheck.cpp
  test/clang-tidy/google-objc-global-variable-declaration.m


Index: clang-tidy/google/GlobalVariableDeclarationCheck.cpp
===
--- clang-tidy/google/GlobalVariableDeclarationCheck.cpp
+++ clang-tidy/google/GlobalVariableDeclarationCheck.cpp
@@ -24,6 +24,10 @@
 
 namespace {
 
+AST_MATCHER(VarDecl, isLocalVariable) {
+  return Node.isLocalVarDecl();
+}
+
 FixItHint generateFixItHint(const VarDecl *Decl, bool IsConst) {
   char FC = Decl->getName()[0];
   if (!llvm::isAlpha(FC) || Decl->getName().size() == 1) {
@@ -57,12 +61,17 @@
   // need to add two matchers since we need to bind different ids to 
distinguish
   // constants and variables. Since bind() can only be called on node matchers,
   // we cannot make it in one matcher.
+  //
+  // Note that hasGlobalStorage() matches static variables declared locally
+  // inside a function or method, so we need to exclude those with
+  // isLocalVariable().
   Finder->addMatcher(
   varDecl(hasGlobalStorage(), unless(hasType(isConstQualified())),
-  unless(matchesName("::g[A-Z]")))
+  unless(isLocalVariable()), unless(matchesName("::g[A-Z]")))
   .bind("global_var"),
   this);
   Finder->addMatcher(varDecl(hasGlobalStorage(), hasType(isConstQualified()),
+ unless(isLocalVariable()),
  unless(matchesName("::k[A-Z]")))
  .bind("global_const"),
  this);
Index: test/clang-tidy/google-objc-global-variable-declaration.m
===
--- test/clang-tidy/google-objc-global-variable-declaration.m
+++ test/clang-tidy/google-objc-global-variable-declaration.m
@@ -30,12 +30,12 @@
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
 static NSString* const kGood = @"hello";
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:24: warning: const global variable 'kGood' 
must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
 static NSString* gMyIntGood = 0;
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:18: warning: non-const global variable 
'gMyIntGood' must have a name which starts with 'g[A-Z]' 
[google-objc-global-variable-declaration]
+
 @implementation Foo
 - (void)f {
 int x = 0;
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:9: warning: non-const global variable 
'gMyIntGood' must have a name which starts with 'g[A-Z]' 
[google-objc-global-variable-declaration]
+static int bar;
+static const int baz = 42;
 }
 @end


Index: clang-tidy/google/GlobalVariableDeclarationCheck.cpp
===
--- clang-tidy/google/GlobalVariableDeclarationCheck.cpp
+++ clang-tidy/google/GlobalVariableDeclarationCheck.cpp
@@ -24,6 +24,10 @@
 
 namespace {
 
+AST_MATCHER(VarDecl, isLocalVariable) {
+  return Node.isLocalVarDecl();
+}
+
 FixItHint generateFixItHint(const VarDecl *Decl, bool IsConst) {
   char FC = Decl->getName()[0];
   if (!llvm::isAlpha(FC) || Decl->getName().size() == 1) {
@@ -57,12 +61,17 @@
   // need to add two matchers since we need to bind different ids to distinguish
   // constants and variables. Since bind() can only be called on node matchers,
   // we cannot make it in one matcher.
+  //
+  // Note that hasGlobalStorage() matches static variables declared locally
+  // inside a function or method, so we need to exclude those with
+  // isLocalVariable().
   Finder->addMatcher(
   varDecl(hasGlobalStorage(), unless(hasType(isConstQualified())),
-  unless(matchesName("::g[A-Z]")))
+  unless(isLocalVariable()), unless(matchesName("::g[A-Z]")))
   .bind("global_var"),
   this);
   Finder->addMatcher(varDecl(hasGlobalStorage(), hasType(isConstQualified()),
+ unless(isLocalVariable()),
  unless(matchesName("::k[A-Z]")))
  .bind("global_const"),
  this);
Index: test/clang-tidy/google-objc-global-variable-declaration.m
===
--- test/clang-tidy/google-objc-global-variable-declaration.m
+++ test/clang-tidy/google-objc-global-variable-declaration.m
@@ -30,12 +30,12 @@
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
 static NSString* const kGood = @"hello";
-// CHECK-MESSAGES-NOT: :[[@LINE-1]]:24: warning: const global variable 'kGood' must have a name which starts with 'k[A-Z]' [google

[PATCH] D41918: [libunwind] Set up .arcconfig to point to new Diffusion UNW repository

[Ben Hamilton via Phabricator via cfe-commits]

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

Too funny. I'll abandon https://reviews.llvm.org/D41917.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D41918



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

[PATCH] D43775: add UUID to the acronyms list of objc property name checks

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton accepted this revision.
benhamilton added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:52
 "GIF",
 "GPS",
 "HD",

Might as well also add GUID.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43775



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

[PATCH] D43732: Resolve build bot problems in unittests/Format/FormatTest.cpp

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Thanks for fixing this. (Sorry for not getting to it, I was away traveling 
until today.)

I'll add a tidy-up in https://reviews.llvm.org/D43598 to ensure all tests which 
call `getStyle()` pass in an in-memory filesystem.


Repository:
  rC Clang

https://reviews.llvm.org/D43732



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

[PATCH] D43732: Resolve build bot problems in unittests/Format/FormatTest.cpp

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Actually, looks like all the other tests explicitly want a non-empty in-memory 
FS, so there's no good cleanup I can do.


Repository:
  rC Clang

https://reviews.llvm.org/D43732



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

[PATCH] D43598: [clang-format] Tidy up new API guessLanguage()

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326205: [clang-format] Tidy up new API guessLanguage() 
(authored by benhamilton, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43598?vs=135341&id=136080#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43598

Files:
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp


Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -2295,8 +2295,8 @@
 }
 
 FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
-  FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
-  if (result == FormatStyle::LK_Cpp) {
+  const auto GuessedLanguage = getLanguageByFileName(FileName);
+  if (GuessedLanguage == FormatStyle::LK_Cpp) {
 auto Extension = llvm::sys::path::extension(FileName);
 // If there's no file extension (or it's .h), we need to check the contents
 // of the code to see if it contains Objective-C.
@@ -2306,12 +2306,11 @@
   Environment::CreateVirtualEnvironment(Code, NonEmptyFileName, 
/*Ranges=*/{});
   ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
   Guesser.process();
-  if (Guesser.isObjC()) {
-result = FormatStyle::LK_ObjC;
-  }
+  if (Guesser.isObjC())
+return FormatStyle::LK_ObjC;
 }
   }
-  return result;
+  return GuessedLanguage;
 }
 
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -11959,33 +11959,15 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
-struct GuessLanguageTestCase {
-  const char *const FileName;
-  const char *const Code;
-  const FormatStyle::LanguageKind ExpectedResult;
-};
-
-class GuessLanguageTest
-: public FormatTest,
-  public ::testing::WithParamInterface {};
-
-TEST_P(GuessLanguageTest, FileAndCode) {
-  auto TestCase = GetParam();
-  EXPECT_EQ(TestCase.ExpectedResult,
-guessLanguage(TestCase.FileName, TestCase.Code));
-}
-
-static const GuessLanguageTestCase TestCases[] = {
-{"foo.cc", "", FormatStyle::LK_Cpp},
-{"foo.m", "", FormatStyle::LK_ObjC},
-{"foo.mm", "", FormatStyle::LK_ObjC},
-{"foo.h", "", FormatStyle::LK_Cpp},
-{"foo.h", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
-{"foo", "", FormatStyle::LK_Cpp},
-{"foo", "@interface Foo\n@end\n", FormatStyle::LK_ObjC},
-};
-INSTANTIATE_TEST_CASE_P(ValidLanguages, GuessLanguageTest,
-::testing::ValuesIn(TestCases));
+TEST_F(FormatTest, FileAndCode) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.cc", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.m", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.mm", ""));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "@interface 
Foo\n@end\n"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo", ""));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface 
Foo\n@end\n"));
+}
 
 } // end namespace
 } // end namespace format


Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -2295,8 +2295,8 @@
 }
 
 FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) {
-  FormatStyle::LanguageKind result = getLanguageByFileName(FileName);
-  if (result == FormatStyle::LK_Cpp) {
+  const auto GuessedLanguage = getLanguageByFileName(FileName);
+  if (GuessedLanguage == FormatStyle::LK_Cpp) {
 auto Extension = llvm::sys::path::extension(FileName);
 // If there's no file extension (or it's .h), we need to check the contents
 // of the code to see if it contains Objective-C.
@@ -2306,12 +2306,11 @@
   Environment::CreateVirtualEnvironment(Code, NonEmptyFileName, /*Ranges=*/{});
   ObjCHeaderStyleGuesser Guesser(*Env, getLLVMStyle());
   Guesser.process();
-  if (Guesser.isObjC()) {
-result = FormatStyle::LK_ObjC;
-  }
+  if (Guesser.isObjC())
+return FormatStyle::LK_ObjC;
 }
   }
-  return result;
+  return GuessedLanguage;
 }
 
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -11959,33 +11959,15 @@
   verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
-struct GuessLanguageTestCase {
-  const char *const FileName;
-  const 

[PATCH] D43775: add UUID to the acronyms list of objc property name checks

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

LGTM.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43775



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

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: krasimir, jolesiak.
Herald added a subscriber: cfe-commits.

Previously, clang-format would detect C++11 and C++17 attribute
specifiers like the following as Objective-C method invocations:

  [[noreturn]];
  [[clang::fallthrough]];
  [[noreturn, deprecated("so sorry")]];
  [[using gsl: suppress("type")]];

To fix this, I ported part of the logic from
tools/clang/lib/Parse/ParseTentative.cpp into TokenAnnotator.cpp so we
can explicitly parse and identify C++11 attribute specifiers.

This allows the guessLanguage() and getStyle() APIs to correctly
guess files containing the C++11 attribute specifiers as C++,
not Objective-C.

Test Plan: New tests added. Ran tests with:

  make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests


Repository:
  rC Clang

https://reviews.llvm.org/D43902

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11969,6 +11969,36 @@
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n@end\n"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "array[[calculator getIndex]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[noreturn, deprecated(\"so sorry\")]];"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage("foo.h", "[[noreturn, deprecated(\"gone, sorry\")]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[noreturn foo] bar];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[clang::fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[clang:fallthrough] foo];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[gsl::suppress(\"type\")]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[using clang: fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[abusing clang:fallthrough] bar];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[using gsl: suppress(\"type\")]];"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage("foo.h",
+"[[clang::callable_when(\"unconsumed\", \"unknown\")]]"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -320,13 +320,65 @@
 return false;
   }
 
+  const FormatToken *parseCpp11Attribute(const FormatToken *Tok,
+ bool NamespaceAllowed) {
+if (!Tok || !Tok->isOneOf(tok::identifier, tok::ellipsis)) return Tok;
+Tok = Tok->Next;
+if (!Tok) return nullptr;
+if (NamespaceAllowed &&
+Tok->startsSequence(tok::coloncolon, tok::identifier)) {
+  Tok = Tok->Next->Next;
+}
+if (!Tok) return nullptr;
+if (Tok->is(tok::l_paren)) {
+  const FormatToken *ParamToken = Tok->Next;
+  while (ParamToken && ParamToken->isNot(tok::r_paren))
+ParamToken = ParamToken->Next;
+  if (!ParamToken || ParamToken->isNot(tok::r_paren)) return nullptr;
+  Tok = ParamToken->Next;
+}
+return Tok;
+  }
+
+  // Look for [[ ... ]] which is a valid C++11 attribute specifier but
+  // never a valid Objective-C or Objective-C++ method invocation.
+  bool parseCpp11AttributeSpecifier(FormatToken *Tok) {
+if (!Style.isCpp()) return false;
+if (!Tok || !Tok->startsSequence(tok::l_square, tok::l_square))
+  return false;
+const FormatToken *AttributeToken = Tok->Next->Next;
+if (!AttributeToken) return false;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'
+if (AttributeToken->startsSequence(tok::kw_using, tok::identifier,
+   tok::colon)) {
+  AttributeToken = AttributeToken->Next->Next->Next;
+  while (AttributeToken) {
+AttributeToken =
+parseCpp11Attribute(AttributeToken, /*NamespaceAllowed=*/false);
+if (!AttributeToken || AttributeToken->isNot(tok::comma)) break;
+AttributeToken = AttributeToken->Next;
+  }
+} else {
+  // C++11 '[[namespace::foo, namespace::bar(baz, blech)]]'
+  while (AttributeToken) {
+AttributeToken =
+parseCpp11Attribute(AttributeToken, /*NamespaceAllowed=*/true);
+if (!AttributeToken || AttributeToken->isNot(tok::comma)) break;
+AttributeToke

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: krasimir, jolesiak.
Herald added a subscriber: cfe-commits.

Previously, clang-format would detect the following as an
Objective-C for-in statement:

  for (int x = in.value(); ...) {}

because the logic only decided a for-loop was definitely *not*
an Objective-C for-in loop after it saw a semicolon or a colon.

To fix this, I delayed the decision of whether this was a for-in
statement until after we found the matching right-paren, at which
point we know if we've seen a semicolon or not.

Test Plan: New tests added. Ran tests with:

  make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests


Repository:
  rC Clang

https://reviews.llvm.org/D43904

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11999,6 +11999,31 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage(
+  "foo.h",
+  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -285,6 +285,16 @@
 else
   Left->PackingKind = PPK_OnePerLine;
 
+if (MightBeObjCForRangeLoop) {
+  FormatToken *ForInToken = Left;
+  while (ForInToken && ForInToken != CurrentToken) {
+if (ForInToken->is(Keywords.kw_in)) {
+  ForInToken->Type = TT_ObjCForIn;
+  break;
+}
+ForInToken = ForInToken->Next;
+  }
+}
 next();
 return true;
   }
@@ -303,8 +313,6 @@
 Contexts.back().IsExpression = false;
   if (CurrentToken->isOneOf(tok::semi, tok::colon))
 MightBeObjCForRangeLoop = false;
-  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in))
-CurrentToken->Type = TT_ObjCForIn;
   // When we discover a 'new', we set CanBeExpression to 'false' in order 
to
   // parse the type correctly. Reset that after a comma.
   if (CurrentToken->is(tok::comma))


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11999,6 +11999,31 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) {}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage(
+  "foo.h",
+  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotat

[PATCH] D43905: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton abandoned this revision.
benhamilton added a comment.

Whoops, sent out twice.


Repository:
  rC Clang

https://reviews.llvm.org/D43905



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

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: krasimir, jolesiak.
Herald added a subscriber: cfe-commits.

Previously, clang-format would detect the following as an
Objective-C block type:

  FOO(^);

when it actually must be a C or C++ macro dealing with an XOR
statement or an XOR operator overload.

According to the Clang Block Language Spec:

https://clang.llvm.org/docs/BlockLanguageSpec.html

block types are of the form:

  int (^)(char, float)

and block variables of block type are of the form:

  void (^blockReturningVoidWithVoidArgument)(void);
  int (^blockReturningIntWithIntAndCharArguments)(int, char);
  void (^arrayOfTenBlocksReturningVoidWithIntArgument[10])(int);

This tightens up the detection so we don't unnecessarily detect
C macros which pass in the XOR operator.

Depends On https://reviews.llvm.org/D43904

Test Plan: New tests added. Ran tests with:

  make -j12 FormatTests &&
  ./tools/clang/unittests/Format/FormatTests


Repository:
  rC Clang

https://reviews.llvm.org/D43906

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12024,6 +12024,16 @@
   "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -142,8 +142,20 @@
 
 bool StartsObjCMethodExpr = false;
 if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
+  const FormatToken *Next = CurrentToken->getNextNonComment();
+  if (Next &&
+  // int (^)(char, float)
+  (Next->startsSequence(tok::r_paren, tok::l_paren) ||
+   // int (^blockReturningIntWithCharAndFloatArguments)(char, float)
+   Next->startsSequence(tok::identifier, tok::r_paren, tok::l_paren) ||
+   // int
+   // 
(^arrayOfTenBlocksReturningIntWithCharAndFloatArguments[10])(char,
+   // float)
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {
+Left->Type = TT_ObjCBlockLParen;
+  }
 } else if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
   if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous 
&&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12024,6 +12024,16 @@
   "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -142,8 +142,20 @@
 
 bool StartsObjCMethodExpr = false;
 if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
+  const FormatToken *Next = CurrentToken->getNextNonComment();
+  if (Next &&
+  // int (^)(char, float)
+  (Next->startsSequence(tok::r_paren, tok::l_paren) ||
+   // int (^blockReturningIntWithCharAndFloatArguments)(char, float)
+   Next->startsSequence(tok::identifier, tok::r_paren, tok::l_paren) ||
+   // int
+   // (^arrayOfTenBlocksReturningIntWithCharAndFloatArguments[10])(char,
+   // float)
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {
+Left->Type = TT_ObjCBlockLParen;
+  }

[PATCH] D43905: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: krasimir, jolesiak.

Previously, clang-format would detect the following as an
Objective-C for-in statement:

  for (int x = in.value(); ...) {}

because the logic only decided a for-loop was definitely *not*
an Objective-C for-in loop after it saw a semicolon or a colon.

To fix this, I delayed the decision of whether this was a for-in
statement until after we found the matching right-paren, at which
point we know if we've seen a semicolon or not.

Depends On https://reviews.llvm.org/D43904

Test Plan: New tests added. Ran tests with:

  make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests


Repository:
  rC Clang

https://reviews.llvm.org/D43905

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11999,6 +11999,31 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage(
+  "foo.h",
+  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -285,6 +285,16 @@
 else
   Left->PackingKind = PPK_OnePerLine;
 
+if (MightBeObjCForRangeLoop) {
+  FormatToken *ForInToken = Left;
+  while (ForInToken && ForInToken != CurrentToken) {
+if (ForInToken->is(Keywords.kw_in)) {
+  ForInToken->Type = TT_ObjCForIn;
+  break;
+}
+ForInToken = ForInToken->Next;
+  }
+}
 next();
 return true;
   }
@@ -303,8 +313,6 @@
 Contexts.back().IsExpression = false;
   if (CurrentToken->isOneOf(tok::semi, tok::colon))
 MightBeObjCForRangeLoop = false;
-  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in))
-CurrentToken->Type = TT_ObjCForIn;
   // When we discover a 'new', we set CanBeExpression to 'false' in order 
to
   // parse the type correctly. Reset that after a comma.
   if (CurrentToken->is(tok::comma))


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11999,6 +11999,31 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) {}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage(
+  "foo.h",
+  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnn

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:346
+  bool parseCpp11AttributeSpecifier(FormatToken *Tok) {
+if (!Style.isCpp()) return false;
+if (!Tok || !Tok->startsSequence(tok::l_square, tok::l_square))

aaron.ballman wrote:
> C can use this syntax as well with `-fdouble-square-bracket-attributes`, 
> which would be good to also support.
Ah, good to know. As far as I know, `clang-format` doesn't actually distinguish 
between C and C++ (or Objective-C and Objective-C++).

In this case, `isCpp()` returns true for all of C, C++, Objective-C, and 
Objective-C++:

https://github.com/llvm-mirror/clang/blob/master/include/clang/Format/Format.h#L1229


Repository:
  rC Clang

https://reviews.llvm.org/D43902



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

[PATCH] D25820: [Sema][Objective-C] Formatting warnings should see through Objective-C message sends

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.
Herald added subscribers: llvm-commits, jkorous-apple.

This caused a regression where `__attribute__((format_arg(X))` no longer works 
with ObjC message sends.

I filed https://bugs.llvm.org/show_bug.cgi?id=36599 and will write a fix.


Repository:
  rL LLVM

https://reviews.llvm.org/D25820



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

[PATCH] D25820: [Sema][Objective-C] Formatting warnings should see through Objective-C message sends

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Actually, looking more deeply, it's possible this is a bug in the Apple SDK:

  @interface NSBundle
  - (NSString *)localizedStringForKey:(NSString *)key value:(nullable NSString 
*)value table:(nullable NSString *)tableName __attribute__((format_arg(1)));
  @end

Since `format_arg(X)` is 1-based, and `value` is the format param here, I 
wonder if this shouldn't be `__attribute__((format_arg(2))`.


Repository:
  rL LLVM

https://reviews.llvm.org/D25820



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

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 137015.
benhamilton marked 7 inline comments as done.
benhamilton added a comment.

Fixes from @jolesiak


Repository:
  rC Clang

https://reviews.llvm.org/D43902

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12080,6 +12080,36 @@
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n@end\n"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "array[[calculator getIndex]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[noreturn, deprecated(\"so sorry\")]];"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage("foo.h", "[[noreturn, deprecated(\"gone, sorry\")]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[noreturn foo] bar];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[clang::fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[clang:fallthrough] foo];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[gsl::suppress(\"type\")]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[using clang: fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[abusing clang:fallthrough] bar];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[using gsl: suppress(\"type\")]];"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage("foo.h",
+"[[clang::callable_when(\"unconsumed\", \"unknown\")]]"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -320,13 +320,68 @@
 return false;
   }
 
+  const FormatToken *parseCpp11Attribute(const FormatToken &Tok,
+ bool NamespaceAllowed) {
+if (!Tok.isOneOf(tok::identifier, tok::ellipsis))
+  return nullptr;
+const FormatToken *AttrTok = Tok.Next;
+if (!AttrTok)
+  return nullptr;
+if (NamespaceAllowed &&
+AttrTok->startsSequence(tok::coloncolon, tok::identifier)) {
+  AttrTok = AttrTok->Next->Next;
+}
+if (!AttrTok)
+  return nullptr;
+if (AttrTok->is(tok::l_paren)) {
+  const FormatToken *ParamToken = AttrTok->Next;
+  while (ParamToken && ParamToken->isNot(tok::r_paren))
+ParamToken = ParamToken->Next;
+  if (!ParamToken)
+return nullptr;
+  AttrTok = ParamToken->Next;
+}
+return AttrTok;
+  }
+
+  // Look for [[ ... ]] which is a valid C++11 attribute specifier but
+  // never a valid Objective-C or Objective-C++ method invocation.
+  bool parseCpp11AttributeSpecifier(const FormatToken &Tok) {
+if (!Style.isCpp())
+  return false;
+if (!Tok.startsSequence(tok::l_square, tok::l_square))
+  return false;
+const FormatToken *AttrTok = Tok.Next->Next;
+if (!AttrTok)
+  return false;
+bool NamespaceAllowed;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'
+if (AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon)) {
+  AttrTok = AttrTok->Next->Next->Next;
+  NamespaceAllowed = false;
+} else {
+  // C++11 '[[namespace::foo, namespace::bar(baz, blech)]]'
+  NamespaceAllowed = true;
+}
+while (AttrTok) {
+  AttrTok = parseCpp11Attribute(*AttrTok, NamespaceAllowed);
+  if (!AttrTok || AttrTok->isNot(tok::comma))
+break;
+  AttrTok = AttrTok->Next;
+}
+if (!AttrTok)
+  return false;
+return AttrTok->startsSequence(tok::r_square, tok::r_square);
+  }
+
   bool parseSquare() {
 if (!CurrentToken)
   return false;
 
 // A '[' could be an index subscript (after an identifier or after
 // ')' or ']'), it could be the start of an Objective-C method
-// expression, or it could the start of an Objective-C array literal.
+// expression, it could the start of an Objective-C array literal,
+// or it could be a C++ attribute specifier [[foo::bar]].
 FormatToken *Left = CurrentToken->Previous;
 Left->ParentBracket = Contexts.back().ContextKind;
 FormatToken *Parent = Left->getPreviousNonComment();
@@ -339,6 +394,12 @@
 (Contexts.back().CanBeExpression || Contexts.back().IsExpression ||
  Contexts.back().InTemplateArgument);
 
+if (parseCpp11AttributeSpecifier(*L

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Thanks very much for the code review. I fixed those issues.




Comment at: lib/Format/TokenAnnotator.cpp:323
 
+  const FormatToken *parseCpp11Attribute(const FormatToken *Tok,
+ bool NamespaceAllowed) {

jolesiak wrote:
> I feel like it would be clearer (and more consistent) if we used const& here 
> and just make a copy inside function body.
> I see no benefit of passing nullptr based on usage presented in this patch.
Fixed.



Comment at: lib/Format/TokenAnnotator.cpp:325
+ bool NamespaceAllowed) {
+if (!Tok || !Tok->isOneOf(tok::identifier, tok::ellipsis)) return Tok;
+Tok = Tok->Next;

jolesiak wrote:
> For consistency reasons I wouldn't use one line ifs. Let's keep existing 
> format with line break and without braces.
Fixed.  (My mistake was using `git-clang-format` without passing `--style llvm` 
— for some reason that's applying a different style.)



Comment at: lib/Format/TokenAnnotator.cpp:337
+ParamToken = ParamToken->Next;
+  if (!ParamToken || ParamToken->isNot(tok::r_paren)) return nullptr;
+  Tok = ParamToken->Next;

jolesiak wrote:
> Is second part of this condition necessary?
Not needed. Removed.



Comment at: lib/Format/TokenAnnotator.cpp:345
+  // never a valid Objective-C or Objective-C++ method invocation.
+  bool parseCpp11AttributeSpecifier(FormatToken *Tok) {
+if (!Style.isCpp()) return false;

jolesiak wrote:
> Pointer should be to const type, but same as above, I feel like const& would 
> be a better choice.
Fixed.



Comment at: lib/Format/TokenAnnotator.cpp:351
+if (!AttributeToken) return false;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'
+if (AttributeToken->startsSequence(tok::kw_using, tok::identifier,

jolesiak wrote:
> How about:
> 
> ```
> bool NamespaceAllowed;
> if (AttributeToken->startsSequence(tok::kw_using, tok::identifier,
>tok::colon)) {
>   // C++17 '[[using namespace: foo, bar(baz, blech)]]'
>   AttributeToken = AttributeToken->Next->Next->Next;
>   NamespaceAllowed = false;
> } else {
>   // C++11 '[[namespace::foo, namespace::bar(baz, blech)]]'
>   NamespaceAllowed = true;
> }
> while (AttributeToken) {
>   AttributeToken =
>   parseCpp11Attribute(AttributeToken, NamespaceAllowed);
>   if (!AttributeToken || AttributeToken->isNot(tok::comma)) break;
>   AttributeToken = AttributeToken->Next;
> }
> ```
Fixed.


Repository:
  rC Clang

https://reviews.llvm.org/D43902



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

[PATCH] D25820: [Sema][Objective-C] Formatting warnings should see through Objective-C message sends

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

> That does look like a bug in the SDK, to me.

OK.  I'll file a Radar bug upstream; in the meantime, this means since this 
diff landed in 2016, clang has been raising `-Wformat-extra-args` when 
compiling any code which passes the result of `-[NSBundle 
localizedStringForKey:value:table:]` to `+[NSString stringWithFormat:]` and 
friends (which is a fairly common operation—any localized string with an 
argument will do this).

I assume Apple will fix this in the SDK the next time they integrate their fork 
of clang from master.


Repository:
  rL LLVM

https://reviews.llvm.org/D25820



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

[PATCH] D25820: [Sema][Objective-C] Formatting warnings should see through Objective-C message sends

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

I filed rdar://38143508 to track this.

The fix won't be easy; I think `__attribute__((format_arg(X)))` isn't enough to 
capture how `-[NSBundle localizedStringForKey:value:table:]` works:

1. `__attribute__((format_arg(X)))` doesn't support multiple format arguments, 
and it appears to ignore everything but the last such declaration:

https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaChecking.cpp#L5030

2. `NSLocalizedString()` and friends pass `@""` as the value argument, which 
raises `-Wformat-zero-length` if we tag it as a format argument:

  test.m:21:12: error: format string is empty [-Werror,-Wformat-zero-length]
@"",
~^~
  test.m:14:46: note: expanded from macro 'NSLocalizedStringWithDefaultValue'
[bundle localizedStringForKey:(key) value:(val) table:(tbl)]
   ^~~
  1 error generated.

This means we'll either need a new attribute which allows a zero-length format 
string, or a way to change format_arg to understand this "argument with default 
value" semantic.


Repository:
  rL LLVM

https://reviews.llvm.org/D25820



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

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 137089.
benhamilton marked 5 inline comments as done.
benhamilton added a comment.

- Fix comments from @krasimir and @djasper.


Repository:
  rC Clang

https://reviews.llvm.org/D43902

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6059,6 +6059,16 @@
AfterType);
 }
 
+TEST_F(FormatTest, UnderstandsSquareAttributes) {
+  verifyFormat("SomeType s [[unused]] (InitValue);");
+  verifyFormat("SomeType s [[gnu::unused]] (InitValue);");
+  verifyFormat("SomeType s [[using gnu: unused]] (InitValue);");
+  verifyFormat("[[gsl::suppress(\"clang-tidy-check-name\")]] void f() {}");
+  verifyFormat("void f() [[deprecated(\"so sorry\")]];");
+  verifyFormat("aa\n"
+   "[[unused]] aaa(int i);");
+}
+
 TEST_F(FormatTest, UnderstandsEllipsis) {
   verifyFormat("int printf(const char *fmt, ...);");
   verifyFormat("template  void Foo(Ts... ts) { Foo(ts...); }");
@@ -12080,6 +12090,36 @@
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n@end\n"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "array[[calculator getIndex]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[noreturn, deprecated(\"so sorry\")]];"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage("foo.h", "[[noreturn, deprecated(\"gone, sorry\")]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[noreturn foo] bar];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[clang::fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[clang:fallthrough] foo];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[gsl::suppress(\"type\")]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[using clang: fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[abusing clang:fallthrough] bar];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[using gsl: suppress(\"type\")]];"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage("foo.h",
+"[[clang::callable_when(\"unconsumed\", \"unknown\")]]"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -320,13 +320,62 @@
 return false;
   }
 
+  const FormatToken *parseCpp11AttributeSpecifier(const FormatToken &Tok) {
+if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
+  return nullptr;
+const FormatToken *AttrTok = Tok.Next->Next;
+if (!AttrTok)
+  return nullptr;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'
+bool IsUsingNamespace =
+AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon);
+if (IsUsingNamespace) {
+  AttrTok = AttrTok->Next->Next->Next;
+}
+auto parseCpp11Attribute = [](const FormatToken &Tok,
+  bool AllowNamespace) -> const FormatToken * {
+  if (!Tok.isOneOf(tok::identifier, tok::ellipsis))
+return nullptr;
+  const FormatToken *AttrTok = Tok.Next;
+  if (!AttrTok)
+return nullptr;
+  if (AllowNamespace &&
+  AttrTok->startsSequence(tok::coloncolon, tok::identifier)) {
+AttrTok = AttrTok->Next->Next;
+  }
+  if (!AttrTok)
+return nullptr;
+  if (AttrTok->is(tok::l_paren)) {
+const FormatToken *ParamToken = AttrTok->Next;
+while (ParamToken && ParamToken->isNot(tok::r_paren))
+  ParamToken = ParamToken->Next;
+if (!ParamToken)
+  return nullptr;
+AttrTok = ParamToken->Next;
+  }
+  return AttrTok;
+};
+while (AttrTok) {
+  AttrTok = parseCpp11Attribute(*AttrTok, !IsUsingNamespace);
+  if (!AttrTok || AttrTok->isNot(tok::comma))
+break;
+  AttrTok = AttrTok->Next;
+}
+if (AttrTok && AttrTok->startsSequence(tok::r_square, tok::r_square)) {
+  return AttrTok->Next;
+} else {
+  return nullptr;
+}
+  }
+
   bool parseSquare() {
 if (!CurrentToken)
   return false;
 
 // A '[' could be an index subscript (after an identifier or after
 // ')' or ']'

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:323
 
+  const FormatToken *parseCpp11Attribute(const FormatToken &Tok,
+ bool NamespaceAllowed) {

krasimir wrote:
> Please inline this into the other function: it's used only once and its name 
> and arguments aren't clear outside of that context.
OK, made this a lambda.



Comment at: lib/Format/TokenAnnotator.cpp:352
+  return false;
+if (!Tok.startsSequence(tok::l_square, tok::l_square))
+  return false;

djasper wrote:
> Just join this if with the previous one.
Fixed.



Comment at: lib/Format/TokenAnnotator.cpp:357
+  return false;
+bool NamespaceAllowed;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'

djasper wrote:
> Replace lines 355-365 with:
> 
>   bool IsUsingNamespace = AttrTok && AttrTok->startsSequence( ... );
>   if (IsUsingNamespace)
> AttrTok = AttrTok->Next->Next->Next;
Fixed.



Comment at: lib/Format/TokenAnnotator.cpp:374
+  return false;
+return AttrTok->startsSequence(tok::r_square, tok::r_square);
+  }

djasper wrote:
> Why not replace these three lines with:
> 
>   return AttrTok && AttrTok->startsSequence(..);
Fixed.



Comment at: lib/Format/TokenAnnotator.cpp:400
+  next();
+  return true;
+}

djasper wrote:
> This seems weird. I think if we return true, we should have consumed 
> everything up to the closing r_square.
That's super useful feedback, I wasn't sure what the contract of this method 
was.

Fixed to consume everything including the closing r_square. (I actually had to 
consume that too, or parsing was left with an un-parsed value and parsing would 
fail.)



Comment at: unittests/Format/FormatTest.cpp:12083
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));

krasimir wrote:
> djasper wrote:
> > You are not adding any test (AFAICT) that tests that you have correctly set 
> > TT_AttributeSpecifier or that you are making any formatting decisions based 
> > on it. If you can't test it, remove that part of this patch.
> This will also require adding tests about not messing up the formatting 
> inside/around attribute specifiers too much. At least take these examples and 
> copy them to the C++/ObjC formatting unittests.
Added formatting tests and logic to use this.

Renamed to `TT_AttributeSquare` to match `TT_AttributeParen`.




Comment at: unittests/Format/FormatTest.cpp:12083
 
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));

benhamilton wrote:
> krasimir wrote:
> > djasper wrote:
> > > You are not adding any test (AFAICT) that tests that you have correctly 
> > > set TT_AttributeSpecifier or that you are making any formatting decisions 
> > > based on it. If you can't test it, remove that part of this patch.
> > This will also require adding tests about not messing up the formatting 
> > inside/around attribute specifiers too much. At least take these examples 
> > and copy them to the C++/ObjC formatting unittests.
> Added formatting tests and logic to use this.
> 
> Renamed to `TT_AttributeSquare` to match `TT_AttributeParen`.
> 
I don't think we have any tests for square-bracket attribute specifier 
formatting (only `__attribute__((foo))`) today. My guess is it's pretty broken.

I've added more tests.



Comment at: unittests/Format/FormatTest.cpp:6068-6069
+  verifyFormat("void f() [[deprecated(\"so sorry\")]];");
+  verifyFormat("aa\n"
+   "[[unused]] aaa(int i);");
+}

I couldn't figure out why this breaks before the `[` in `[[unused]]`, when the 
identical test using `__attribute__((unused))` above does *not* break before 
the `__attribute__`.

I ran with `-debug` and the two seem fairly similar. Can anyone help shed light 
on this?

## With `__attribute__((unused))`

P8067

## With `[[unused]]`

P8068


Repository:
  rC Clang

https://reviews.llvm.org/D43902



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

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 137092.
benhamilton marked an inline comment as done.
benhamilton added a comment.

- Fix comments from @djasper and @krasimir.


Repository:
  rC Clang

https://reviews.llvm.org/D43904

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -775,6 +775,20 @@
" .().a().a()) {\n}");
 }
 
+TEST_F(FormatTest, ObjCForInLoop) {
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");
+  verifyFormat("for (Foo *x in bar) {\n}");
+  verifyFormat("for (Foo *x in [bar baz]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:^{\n"
+   "   [uh oh];\n"
+   " }]) {\n}");
+  verifyFormat("Foo *x;\nfor (x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x in y) {\n}");
+  verifyFormat("for (const Foo &baz = in.value(); !baz.at_end(); ++baz) 
{\n}");
+}
+
 TEST_F(FormatTest, ForEachLoops) {
   verifyFormat("void f() {\n"
"  foreach (Item *item, itemlist) {}\n"
@@ -12120,6 +12134,31 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage(
+  "foo.h",
+  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -216,6 +216,7 @@
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
 Left->Previous && Left->Previous->is(tok::kw_for);
+FormatToken *PossibleObjCForInToken = nullptr;
 while (CurrentToken) {
   // LookForDecls is set when "if (" has been seen. Check for
   // 'identifier' '*' 'identifier' followed by not '=' -- this
@@ -301,10 +302,15 @@
CurrentToken->Previous->isSimpleTypeSpecifier()) &&
   !CurrentToken->is(tok::l_brace))
 Contexts.back().IsExpression = false;
-  if (CurrentToken->isOneOf(tok::semi, tok::colon))
+  if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
 MightBeObjCForRangeLoop = false;
-  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in))
-CurrentToken->Type = TT_ObjCForIn;
+if (PossibleObjCForInToken)
+  PossibleObjCForInToken->Type = TT_Unknown;
+  }
+  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in)) {
+PossibleObjCForInToken = CurrentToken;
+PossibleObjCForInToken->Type = TT_ObjCForIn;
+  }
   // When we discover a 'new', we set CanBeExpression to 'false' in order 
to
   // parse the type correctly. Reset that after a comma.
   if (CurrentToken->is(tok::comma))


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -775,6 +775,20 @@
" .().a().a()) {\n}");
 }
 
+TEST_F(FormatTest, ObjCForInLoop) {
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");
+  verifyFormat("for (Foo *x in bar) {\n}");
+  verifyFormat("for (Foo *x in [bar baz]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:^{\n"
+   "   [uh oh];\n"
+   " }]) {\n}");
+  verifyFormat("Foo *x;\nfor (x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x in y) {\n}");
+  verifyFormat("for (const Foo &baz = in.value(); !baz.at_end(); ++baz) {\n}");
+}
+
 TEST_F(Form

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

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

One more cleanup


Repository:
  rC Clang

https://reviews.llvm.org/D43904

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -775,6 +775,20 @@
" .().a().a()) {\n}");
 }
 
+TEST_F(FormatTest, ObjCForInLoop) {
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");
+  verifyFormat("for (Foo *x in bar) {\n}");
+  verifyFormat("for (Foo *x in [bar baz]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:^{\n"
+   "   [uh oh];\n"
+   " }]) {\n}");
+  verifyFormat("Foo *x;\nfor (x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x in y) {\n}");
+  verifyFormat("for (const Foo &baz = in.value(); !baz.at_end(); ++baz) 
{\n}");
+}
+
 TEST_F(FormatTest, ForEachLoops) {
   verifyFormat("void f() {\n"
"  foreach (Item *item, itemlist) {}\n"
@@ -12120,6 +12134,31 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage(
+  "foo.h",
+  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -216,6 +216,7 @@
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
 Left->Previous && Left->Previous->is(tok::kw_for);
+FormatToken *PossibleObjCForInToken = nullptr;
 while (CurrentToken) {
   // LookForDecls is set when "if (" has been seen. Check for
   // 'identifier' '*' 'identifier' followed by not '=' -- this
@@ -301,10 +302,17 @@
CurrentToken->Previous->isSimpleTypeSpecifier()) &&
   !CurrentToken->is(tok::l_brace))
 Contexts.back().IsExpression = false;
-  if (CurrentToken->isOneOf(tok::semi, tok::colon))
+  if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
 MightBeObjCForRangeLoop = false;
-  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in))
-CurrentToken->Type = TT_ObjCForIn;
+if (PossibleObjCForInToken) {
+  PossibleObjCForInToken->Type = TT_Unknown;
+  PossibleObjCForInToken = nullptr;
+}
+  }
+  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in)) {
+PossibleObjCForInToken = CurrentToken;
+PossibleObjCForInToken->Type = TT_ObjCForIn;
+  }
   // When we discover a 'new', we set CanBeExpression to 'false' in order 
to
   // parse the type correctly. Reset that after a comma.
   if (CurrentToken->is(tok::comma))


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -775,6 +775,20 @@
" .().a().a()) {\n}");
 }
 
+TEST_F(FormatTest, ObjCForInLoop) {
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");
+  verifyFormat("for (Foo *x in bar) {\n}");
+  verifyFormat("for (Foo *x in [bar baz]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:^{\n"
+   "   [uh oh];\n"
+   " }]) {\n}");
+  verifyFormat("Foo *x;\nfor (x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x in y) {\n}");
+  verifyFormat("for (const Foo &baz = in.value(); !baz.at_end(); ++baz) {\n}");
+}
+
 TEST_F(FormatTest, ForEach

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:288
 
+if (MightBeObjCForRangeLoop) {
+  FormatToken *ForInToken = Left;

djasper wrote:
> There can be only one ObjCForIn token in any for loop, right? If that's the 
> case, can we just remember that in addition to (or instead of) 
> MightBeObjCForRangeLoop? That way, we wouldn't have to re-iterate over all 
> the tokens here, but could just set this directly.
Nice optimization, done.



Comment at: unittests/Format/FormatTest.cpp:12002
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,

krasimir wrote:
> Please also add this instances as formatting tests.
Thanks, tests added.

The new formatting tests revealed a new regression I'd introduced: we were not 
inserting a space between keyword 'in' and the opening `[` of the ObjC message 
send.

This was because the ObjC message send parsing logic depended on the 
TT_ObjCForIn keyword being set before parsing the `[`, but by delaying setting 
the type until after parsing the for-loop was complete, we broke that logic.

Fixed regression and made sure tests passed.


Repository:
  rC Clang

https://reviews.llvm.org/D43904



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

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 137095.
benhamilton marked 2 inline comments as done.
benhamilton added a comment.

- Fix comments from @djasper and @krasimir.


Repository:
  rC Clang

https://reviews.llvm.org/D43906

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7695,6 +7695,11 @@
   verifyGoogleFormat("- foo:(int)foo;");
 }
 
+TEST_F(FormatTest, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+}
 
 TEST_F(FormatTest, BreaksStringLiterals) {
   EXPECT_EQ("\"some text \"\n"
@@ -12159,6 +12164,16 @@
   "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -142,8 +142,20 @@
 
 bool StartsObjCMethodExpr = false;
 if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
+  const FormatToken *Next = CurrentToken->getNextNonComment();
+  if (Next &&
+  // int (^)(char, float)
+  (Next->startsSequence(tok::r_paren, tok::l_paren) ||
+   // int (^blockReturningIntWithCharAndFloatArguments)(char, float)
+   Next->startsSequence(tok::identifier, tok::r_paren, tok::l_paren) ||
+   // int
+   // 
(^arrayOfTenBlocksReturningIntWithCharAndFloatArguments[10])(char,
+   // float)
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {
+Left->Type = TT_ObjCBlockLParen;
+  }
 } else if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
   if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous 
&&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -7695,6 +7695,11 @@
   verifyGoogleFormat("- foo:(int)foo;");
 }
 
+TEST_F(FormatTest, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+}
 
 TEST_F(FormatTest, BreaksStringLiterals) {
   EXPECT_EQ("\"some text \"\n"
@@ -12159,6 +12164,16 @@
   "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -142,8 +142,20 @@
 
 bool StartsObjCMethodExpr = false;
 if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
+  const FormatToken *Next = CurrentToken->getNextNonComment();
+  if (Next &&
+  // int (^)(char, float)
+  (Next->startsSequence(tok::r_paren, tok::l_paren) ||
+   // int (^blockReturningIntWithCharAndFloatArguments)(char, float)
+   Next->startsSequence(tok::identifier, tok::r_paren, tok::l_paren) ||
+   // int
+   // (^arrayOfTenBlocksReturningIntWithCharAndFloatArguments[10])(char,
+   // float)
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {
+Left->Type = TT_ObjCBlockLParen;
+  }
 } else if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a sel

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

> Would it be enough to only add the block type case? With the macro false 
> positive, there won't be an open paren after the closing paren, right?

Are you asking to remove the block variable cases? I was concerned those would 
no longer be handled correctly if we don't explicitly check for them.

What change are you suggesting if we remove the block variable cases to handle 
those?




Comment at: lib/Format/TokenAnnotator.cpp:155
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {

djasper wrote:
> This seems suspect. Does it have to be a numeric_constant?
Probably not, any constexpr would do, I suspect. What's the best way to parse 
that?



Comment at: unittests/Format/FormatTest.cpp:12027
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));

krasimir wrote:
> Please also add formatting tests. This might require changes to the 
> formatting logic since I'd intuitively expect ``int(^)(char)`` to be 
> formatted as ``int (^)(char)``.
Added formatting tests. Tests passed with no changes to the formatting logic, 
and your intuition was correct.



Comment at: unittests/Format/FormatTest.cpp:12029
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));

djasper wrote:
> I am somewhat skeptical about all these tests now being solely about 
> guessLanguage. It might be the best choice for some of them, but also, there 
> might be other things we do to detect ObjC at some point and then all of 
> these tests become meaningless.
> 
> Does your change create a different formatting here?
I hear you. I just want to make sure the false positive case doesn't regress, 
and we don't lose the functionality of detecting ObjC for block types.

Added formatting tests.


Repository:
  rC Clang

https://reviews.llvm.org/D43906



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

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:155
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {

djasper wrote:
> benhamilton wrote:
> > djasper wrote:
> > > This seems suspect. Does it have to be a numeric_constant?
> > Probably not, any constexpr would do, I suspect. What's the best way to 
> > parse that?
> I think this is the same answer for both of your questions. If what you are 
> trying to prevent "FOO(^)" to be parsed as a block, wouldn't it be enough to 
> look for whether there is a "(" after the ")" or even only after "(^)", 
> everything else is already correct IIUC? That would get you out of need to 
> parse the specifics here, which will be hard.
> 
> Or thinking about it another way. Previously, every "(^" would be parsed as 
> an ObjC block. There seems to be only a really rare corner case in which it 
> isn't (macros). Thus, I'd just try to detect that corner case. Instead you 
> are completely inverting the defaults (defaulting to "^" is not a block) and 
> then try to exactly parse ObjC where there might be many cases and edge cases 
> that you won't even think of now.
Hmm. Well, it's not just `FOO(^);` that isn't a block:

```
#define FOO(X) operator X

SomeType FOO(^)(int x, const SomeType& y) { ... }
```

Obviously we can't get this perfect without a pre-processor, but it seems like 
our best bet is to only assign mark `TT_ObjCBlockLParen` when we are sure the 
syntax is a valid block type or block variable.


Repository:
  rC Clang

https://reviews.llvm.org/D43906



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

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 137199.
benhamilton marked an inline comment as done.
benhamilton added a comment.

- Move ObjC-specific tests to `FormatTestObjC.cpp`.


Repository:
  rC Clang

https://reviews.llvm.org/D43904

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -893,6 +893,13 @@
"foo(n);\n"
"  }\n"
"}");
+  verifyFormat("for (Foo *x in bar) {\n}");
+  verifyFormat("for (Foo *x in [bar baz]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:^{\n"
+   "   [uh oh];\n"
+   " }]) {\n}");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -742,6 +742,12 @@
" aaa != bbb;\n"
" ++aaa) {");
 
+  // These should not be formatted as Objective-C for-in loops.
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x in y) {\n}");
+  verifyFormat("for (const Foo &baz = in.value(); !baz.at_end(); ++baz) 
{\n}");
+
   FormatStyle NoBinPacking = getLLVMStyle();
   NoBinPacking.BinPackParameters = false;
   verifyFormat("for (int aaa = 1;\n"
@@ -12120,6 +12126,31 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage(
+  "foo.h",
+  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -216,6 +216,7 @@
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
 Left->Previous && Left->Previous->is(tok::kw_for);
+FormatToken *PossibleObjCForInToken = nullptr;
 while (CurrentToken) {
   // LookForDecls is set when "if (" has been seen. Check for
   // 'identifier' '*' 'identifier' followed by not '=' -- this
@@ -301,10 +302,17 @@
CurrentToken->Previous->isSimpleTypeSpecifier()) &&
   !CurrentToken->is(tok::l_brace))
 Contexts.back().IsExpression = false;
-  if (CurrentToken->isOneOf(tok::semi, tok::colon))
+  if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
 MightBeObjCForRangeLoop = false;
-  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in))
-CurrentToken->Type = TT_ObjCForIn;
+if (PossibleObjCForInToken) {
+  PossibleObjCForInToken->Type = TT_Unknown;
+  PossibleObjCForInToken = nullptr;
+}
+  }
+  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in)) {
+PossibleObjCForInToken = CurrentToken;
+PossibleObjCForInToken->Type = TT_ObjCForIn;
+  }
   // When we discover a 'new', we set CanBeExpression to 'false' in order 
to
   // parse the type correctly. Reset that after a comma.
   if (CurrentToken->is(tok::comma))


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -893,6 +893,13 @@
"foo(n);\n"
"  }\n"
"}");
+  verifyFormat("for (Foo *x in bar) {\n}");
+  verifyFormat("for (Foo *x in [bar baz]) {\

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: unittests/Format/FormatTest.cpp:778
 
+TEST_F(FormatTest, ObjCForInLoop) {
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");

krasimir wrote:
> Please move the ObjC-specific instances to `FormatTestObjC.cpp`.
Done.


Repository:
  rC Clang

https://reviews.llvm.org/D43904



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

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:155
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {

benhamilton wrote:
> djasper wrote:
> > benhamilton wrote:
> > > djasper wrote:
> > > > This seems suspect. Does it have to be a numeric_constant?
> > > Probably not, any constexpr would do, I suspect. What's the best way to 
> > > parse that?
> > I think this is the same answer for both of your questions. If what you are 
> > trying to prevent "FOO(^)" to be parsed as a block, wouldn't it be enough 
> > to look for whether there is a "(" after the ")" or even only after "(^)", 
> > everything else is already correct IIUC? That would get you out of need to 
> > parse the specifics here, which will be hard.
> > 
> > Or thinking about it another way. Previously, every "(^" would be parsed as 
> > an ObjC block. There seems to be only a really rare corner case in which it 
> > isn't (macros). Thus, I'd just try to detect that corner case. Instead you 
> > are completely inverting the defaults (defaulting to "^" is not a block) 
> > and then try to exactly parse ObjC where there might be many cases and edge 
> > cases that you won't even think of now.
> Hmm. Well, it's not just `FOO(^);` that isn't a block:
> 
> ```
> #define FOO(X) operator X
> 
> SomeType FOO(^)(int x, const SomeType& y) { ... }
> ```
> 
> Obviously we can't get this perfect without a pre-processor, but it seems 
> like our best bet is to only assign mark `TT_ObjCBlockLParen` when we are 
> sure the syntax is a valid block type or block variable.
I tried the suggestion to only treat `(^)(` as a block type, but it appears 
this is the primary place where we set `TT_ObjCBlockLParen`, so I think we 
really do need to handle the other cases here.


Repository:
  rC Clang

https://reviews.llvm.org/D43906



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

[PATCH] D43904: [clang-format] Improve detection of ObjC for-in statements

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326815: [clang-format] Improve detection of ObjC for-in 
statements (authored by benhamilton, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43904?vs=137199&id=137218#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43904

Files:
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp
  cfe/trunk/unittests/Format/FormatTestObjC.cpp


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -216,6 +216,7 @@
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
 Left->Previous && Left->Previous->is(tok::kw_for);
+FormatToken *PossibleObjCForInToken = nullptr;
 while (CurrentToken) {
   // LookForDecls is set when "if (" has been seen. Check for
   // 'identifier' '*' 'identifier' followed by not '=' -- this
@@ -301,10 +302,17 @@
CurrentToken->Previous->isSimpleTypeSpecifier()) &&
   !CurrentToken->is(tok::l_brace))
 Contexts.back().IsExpression = false;
-  if (CurrentToken->isOneOf(tok::semi, tok::colon))
+  if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
 MightBeObjCForRangeLoop = false;
-  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in))
-CurrentToken->Type = TT_ObjCForIn;
+if (PossibleObjCForInToken) {
+  PossibleObjCForInToken->Type = TT_Unknown;
+  PossibleObjCForInToken = nullptr;
+}
+  }
+  if (MightBeObjCForRangeLoop && CurrentToken->is(Keywords.kw_in)) {
+PossibleObjCForInToken = CurrentToken;
+PossibleObjCForInToken->Type = TT_ObjCForIn;
+  }
   // When we discover a 'new', we set CanBeExpression to 'false' in order 
to
   // parse the type correctly. Reset that after a comma.
   if (CurrentToken->is(tok::comma))
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -742,6 +742,12 @@
" aaa != bbb;\n"
" ++aaa) {");
 
+  // These should not be formatted as Objective-C for-in loops.
+  verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x = 0; x != in; x++) {\n}");
+  verifyFormat("Foo *x;\nfor (x in y) {\n}");
+  verifyFormat("for (const Foo &baz = in.value(); !baz.at_end(); ++baz) 
{\n}");
+
   FormatStyle NoBinPacking = getLLVMStyle();
   NoBinPacking.BinPackParameters = false;
   verifyFormat("for (int aaa = 1;\n"
@@ -12082,6 +12088,31 @@
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface 
Foo\n@end\n"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithForIn) {
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in bar) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) 
{}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+  EXPECT_EQ(
+  FormatStyle::LK_Cpp,
+  guessLanguage(
+  "foo.h",
+  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -893,6 +893,13 @@
"foo(n);\n"
"  }\n"
"}");
+  verifyFormat("for (Foo *x in bar) {\n}");
+  verifyFormat("for (Foo *x in [bar baz]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {\n}");
+  verifyFormat("for (Foo *x in [bar baz:^{\n"
+   "   [uh oh];\n"
+   " }]) {\n}");
 }
 
 TEST_F(FormatTestObjC, ObjCLiterals) {


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:155
+   Next->startsSequence(tok::identifier, tok::l_square,
+tok::numeric_constant, tok::r_square,
+tok::r_paren, tok::l_paren))) {

djasper wrote:
> benhamilton wrote:
> > benhamilton wrote:
> > > djasper wrote:
> > > > benhamilton wrote:
> > > > > djasper wrote:
> > > > > > This seems suspect. Does it have to be a numeric_constant?
> > > > > Probably not, any constexpr would do, I suspect. What's the best way 
> > > > > to parse that?
> > > > I think this is the same answer for both of your questions. If what you 
> > > > are trying to prevent "FOO(^)" to be parsed as a block, wouldn't it be 
> > > > enough to look for whether there is a "(" after the ")" or even only 
> > > > after "(^)", everything else is already correct IIUC? That would get 
> > > > you out of need to parse the specifics here, which will be hard.
> > > > 
> > > > Or thinking about it another way. Previously, every "(^" would be 
> > > > parsed as an ObjC block. There seems to be only a really rare corner 
> > > > case in which it isn't (macros). Thus, I'd just try to detect that 
> > > > corner case. Instead you are completely inverting the defaults 
> > > > (defaulting to "^" is not a block) and then try to exactly parse ObjC 
> > > > where there might be many cases and edge cases that you won't even 
> > > > think of now.
> > > Hmm. Well, it's not just `FOO(^);` that isn't a block:
> > > 
> > > ```
> > > #define FOO(X) operator X
> > > 
> > > SomeType FOO(^)(int x, const SomeType& y) { ... }
> > > ```
> > > 
> > > Obviously we can't get this perfect without a pre-processor, but it seems 
> > > like our best bet is to only assign mark `TT_ObjCBlockLParen` when we are 
> > > sure the syntax is a valid block type or block variable.
> > I tried the suggestion to only treat `(^)(` as a block type, but it appears 
> > this is the primary place where we set `TT_ObjCBlockLParen`, so I think we 
> > really do need to handle the other cases here.
> I don't follow your logic. I'd like you to slowly change this as opposed to 
> completely going the opposite way.
> 
> So currently, the only know real-live problem is "FOO(^);". So address this 
> somehow, but still default/error to recognizing too much stuff as a block.
> 
> Have you actually seen
> 
>   SomeType FOO(^)(int x, const SomeType& y) { ... }
> 
> in real code?
I haven't seen that example, but I have seen `FOO(^, OtherParam);`.

I'm trying to change the parser to handle this now without explicitly parsing 
all block types, but it's tricky. The following should be ObjC block types:

```
(^)(int, char);
(^foo)(int, char);
(^foo[10])(int, char);
(^foo[kNum])(int, char);
(^foo[(kNum + kOtherNum)])(int, char);
```

but the following should not:

```
FOO(^);
FOO(^, int, char);
```

I'll make it work, it's just easy to make a mistake.


Repository:
  rC Clang

https://reviews.llvm.org/D43906



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

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

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

- Simplify logic and assume we have an ObjC block whenever we see rparen at the 
end of `(^...)(`.


Repository:
  rC Clang

https://reviews.llvm.org/D43906

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
"  foo();\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12128,6 +12128,22 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^, Bar);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[kNumEntries])(char, float);"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -142,8 +142,32 @@
 
 bool StartsObjCMethodExpr = false;
 if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
+  // The following are valid block ObjC types and block variables:
+  //
+  // int (^)(char, float)
+  // int (^block)(char, float)
+  // int (^arrayOfTenBlocks[10])(char, float)
+  // int (^arrayOfManyBlocks[(kLen + 42)])(char, float)
+  const FormatToken *Next = CurrentToken->getNextNonComment();
+  int ParenDepth = 1;
+  // Handle nested parens in case we have an array of blocks with
+  // a parenthesized length.
+  while (Next) {
+if (Next->is(tok::l_paren))
+  ++ParenDepth;
+else if (Next->is(tok::r_paren))
+  --ParenDepth;
+if (ParenDepth == 0)
+  break;
+Next = Next->getNextNonComment();
+  }
+  if (Next) {
+const FormatToken *NextNext = Next->getNextNonComment();
+// If we have an opening paren at the end of (^...)( we assume
+// we have an Objective-C block.
+if (NextNext && NextNext->is(tok::l_paren))
+  Left->Type = TT_ObjCBlockLParen;
+  }
 } else if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
   if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous 
&&


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
"  foo();\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12128,6 +12128,22 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

> Right. So the difference is that for blocks, there is always a "(" after the 
> "(^.)". That should be easy to parse, no?

Yep, I've updated this diff to implement that as you recommended. I just had to 
handle nested parens (I couldn't find an existing way to deal with these so I 
wrote my own—let me know if I missed something.)


Repository:
  rC Clang

https://reviews.llvm.org/D43906



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

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

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

Fix comment.


Repository:
  rC Clang

https://reviews.llvm.org/D43906

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
"  foo();\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12128,6 +12128,22 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^, Bar);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[kNumEntries])(char, float);"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -142,8 +142,32 @@
 
 bool StartsObjCMethodExpr = false;
 if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
+  // The following are valid ObjC block types and block variables:
+  //
+  // int (^)(char, float)
+  // int (^block)(char, float)
+  // int (^arrayOfTenBlocks[10])(char, float)
+  // int (^arrayOfManyBlocks[(kLen + 42)])(char, float)
+  const FormatToken *Next = CurrentToken->getNextNonComment();
+  int ParenDepth = 1;
+  // Handle nested parens in case we have an array of blocks with
+  // a parenthesized length.
+  while (Next) {
+if (Next->is(tok::l_paren))
+  ++ParenDepth;
+else if (Next->is(tok::r_paren))
+  --ParenDepth;
+if (ParenDepth == 0)
+  break;
+Next = Next->getNextNonComment();
+  }
+  if (Next) {
+const FormatToken *NextNext = Next->getNextNonComment();
+// If we have an opening paren at the end of (^...)( we assume
+// we have an Objective-C block.
+if (NextNext && NextNext->is(tok::l_paren))
+  Left->Type = TT_ObjCBlockLParen;
+  }
 } else if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
   if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous 
&&


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
"  foo();\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12128,6 +12128,22 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^, Bar);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "in

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

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

- Greatly clean up ObjC block type logic by re-using `TT_FunctionTypeLParen` 
type logic.


Repository:
  rC Clang

https://reviews.llvm.org/D43906

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
"  foo();\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12128,6 +12128,22 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^, Bar);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[kNumEntries])(char, float);"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -141,10 +141,7 @@
 Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
 bool StartsObjCMethodExpr = false;
-if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
-} else if (FormatToken *MaybeSel = Left->Previous) {
+if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
   if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous 
&&
   MaybeSel->Previous->is(tok::at)) {
@@ -210,8 +207,10 @@
   Left->Type = TT_ObjCMethodExpr;
 }
 
-bool MightBeFunctionType = !Contexts[Contexts.size() - 2].IsExpression;
-bool ProbablyFunctionType = CurrentToken->isOneOf(tok::star, tok::amp);
+bool MightBeFunctionOrObjCBlockType =
+!Contexts[Contexts.size() - 2].IsExpression;
+bool ProbablyFunctionOrObjCBlockType =
+CurrentToken->isOneOf(tok::star, tok::amp, tok::caret);
 bool HasMultipleLines = false;
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
@@ -239,16 +238,18 @@
   if (CurrentToken->Previous->is(TT_PointerOrReference) &&
   CurrentToken->Previous->Previous->isOneOf(tok::l_paren,
 tok::coloncolon))
-ProbablyFunctionType = true;
+ProbablyFunctionOrObjCBlockType = true;
   if (CurrentToken->is(tok::comma))
-MightBeFunctionType = false;
+MightBeFunctionOrObjCBlockType = false;
   if (CurrentToken->Previous->is(TT_BinaryOperator))
 Contexts.back().IsExpression = true;
   if (CurrentToken->is(tok::r_paren)) {
-if (MightBeFunctionType && ProbablyFunctionType && CurrentToken->Next 
&&
+if (MightBeFunctionOrObjCBlockType && ProbablyFunctionOrObjCBlockType 
&&
+CurrentToken->Next &&
 (CurrentToken->Next->is(tok::l_paren) ||
  (CurrentToken->Next->is(tok::l_square) && 
Line.MustBeDeclaration)))
-  Left->Type = TT_FunctionTypeLParen;
+  Left->Type = Left->Next->is(tok::caret) ? TT_ObjCBlockLParen
+  : TT_FunctionTypeLParen;
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
 


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void Do

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:152
+  const FormatToken *Next = CurrentToken->getNextNonComment();
+  int ParenDepth = 1;
+  // Handle nested parens in case we have an array of blocks with

djasper wrote:
> No. Don't implement yet another parenthesis counting. This function already 
> iterates over all tokens until the closing paren. Just store a pointer to the 
> caret here and mark it (or unmark it) when encountering the closing brace 
> (line 271). There is already very similar logic there to set 
> TT_FunctionTypeLParen (which is actually doing the exact same parsing now 
> that I think of it).
Oh! Yes, re-using the logic for `TT_FunctionTypeLParen` is clearly the right 
thing to do, since it has exactly the same syntax (just `*` or `&` instead of 
`^`).

Done, code is much cleaner now.


Repository:
  rC Clang

https://reviews.llvm.org/D43906



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

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

I made it longer so the two lines (__attribute__((foo)) vs. [[foo]]) had
the same length.

If I shorten it much more, everything goes on one line.


Repository:
  rC Clang

https://reviews.llvm.org/D43902



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

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton updated this revision to Diff 137696.
benhamilton marked 9 inline comments as done.
benhamilton added a comment.

Refactor to avoid matching open and close parens. Fix @djasper comments.


Repository:
  rC Clang

https://reviews.llvm.org/D43902

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6067,6 +6067,16 @@
AfterType);
 }
 
+TEST_F(FormatTest, UnderstandsSquareAttributes) {
+  verifyFormat("SomeType s [[unused]] (InitValue);");
+  verifyFormat("SomeType s [[gnu::unused]] (InitValue);");
+  verifyFormat("SomeType s [[using gnu: unused]] (InitValue);");
+  verifyFormat("[[gsl::suppress(\"clang-tidy-check-name\")]] void f() {}");
+  verifyFormat("void f() [[deprecated(\"so sorry\")]];");
+  verifyFormat("aa\n"
+   "[[unused]] aaa(int i);");
+}
+
 TEST_F(FormatTest, UnderstandsEllipsis) {
   verifyFormat("int printf(const char *fmt, ...);");
   verifyFormat("template  void Foo(Ts... ts) { Foo(ts...); }");
@@ -12088,29 +12098,34 @@
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n@end\n"));
 }
 
-TEST_F(FormatTest, GuessLanguageWithForIn) {
-  EXPECT_EQ(FormatStyle::LK_Cpp,
-guessLanguage("foo.h", "for (Foo *x = 0; x != in; x++) {}"));
+TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[noreturn]];"));
   EXPECT_EQ(FormatStyle::LK_ObjC,
-guessLanguage("foo.h", "for (Foo *x in bar) {}"));
-  EXPECT_EQ(FormatStyle::LK_ObjC,
-guessLanguage("foo.h", "for (Foo *x in [bar baz]) {}"));
-  EXPECT_EQ(FormatStyle::LK_ObjC,
-guessLanguage("foo.h", "for (Foo *x in [bar baz:blech]) {}"));
+guessLanguage("foo.h", "array[[calculator getIndex]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[noreturn, deprecated(\"so sorry\")]];"));
   EXPECT_EQ(
-  FormatStyle::LK_ObjC,
-  guessLanguage("foo.h", "for (Foo *x in [bar baz:blech, 1, 2, 3, 0]) {}"));
+  FormatStyle::LK_Cpp,
+  guessLanguage("foo.h", "[[noreturn, deprecated(\"gone, sorry\")]];"));
   EXPECT_EQ(FormatStyle::LK_ObjC,
-guessLanguage("foo.h", "for (Foo *x in [bar baz:^{[uh oh];}]) {}"));
+guessLanguage("foo.h", "[[noreturn foo] bar];"));
   EXPECT_EQ(FormatStyle::LK_Cpp,
-guessLanguage("foo.h", "Foo *x; for (x = 0; x != in; x++) {}"));
+guessLanguage("foo.h", "[[clang::fallthrough]];"));
   EXPECT_EQ(FormatStyle::LK_ObjC,
-guessLanguage("foo.h", "Foo *x; for (x in y) {}"));
+guessLanguage("foo.h", "[[clang:fallthrough] foo];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[gsl::suppress(\"type\")]];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[using clang: fallthrough]];"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "[[abusing clang:fallthrough] bar];"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "[[using gsl: suppress(\"type\")]];"));
   EXPECT_EQ(
   FormatStyle::LK_Cpp,
-  guessLanguage(
-  "foo.h",
-  "for (const Foo& baz = in.value(); !baz.at_end(); ++baz) {}"));
+  guessLanguage("foo.h",
+"[[clang::callable_when(\"unconsumed\", \"unknown\")]]"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
 } // end namespace
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -328,13 +328,40 @@
 return false;
   }
 
+  bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
+if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
+  return false;
+const FormatToken *AttrTok = Tok.Next->Next;
+if (!AttrTok)
+  return false;
+// C++17 '[[using ns: foo, bar(baz, blech)]]'
+// We assume nobody will name an ObjC variable 'using'.
+if (AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon))
+  return true;
+if (AttrTok->isNot(tok::identifier))
+  return false;
+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(:)]]
+  if (AttrTok->is(tok::colon) ||
+  AttrTok->startsSequence(tok::identifier, tok::identifier))
+return false;
+  if (AttrTok->is(tok::ellipsis))
+return true;
+  AttrTok = AttrTok->Next;
+}
+

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Great, I refactored it to avoid parsing the matching parens, so there's no more 
danger of O(N^2) parsing behavior.

I had to add a new `InCpp11AttributeSpecifier` boolean to `Context` so when 
parsing the inner `[]` we didn't treat it as an ObjC message send or an array 
subscript.




Comment at: lib/Format/TokenAnnotator.cpp:329
+  return nullptr;
+// C++17 '[[using namespace: foo, bar(baz, blech)]]'
+bool IsUsingNamespace =

djasper wrote:
> Can you  make this:
> 
>   // C++17 '[[using : foo, bar(baz, blech)]]'
> 
> To make clear that we are not looking for kw_namespace here?
Ah, good call. Done.



Comment at: lib/Format/TokenAnnotator.cpp:332
+AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon);
+if (IsUsingNamespace) {
+  AttrTok = AttrTok->Next->Next->Next;

djasper wrote:
> No braces.
Done.



Comment at: lib/Format/TokenAnnotator.cpp:336
+auto parseCpp11Attribute = [](const FormatToken &Tok,
+  bool AllowNamespace) -> const FormatToken * {
+  if (!Tok.isOneOf(tok::identifier, tok::ellipsis))

djasper wrote:
> Do you actually need to put the return type here? I would have thought that 
> it can be deduced as you pass back a const FormatToken* from a codepath and 
> nullptr from all the others.
Looks like we don't. Fixed.



Comment at: lib/Format/TokenAnnotator.cpp:342
+return nullptr;
+  if (AllowNamespace &&
+  AttrTok->startsSequence(tok::coloncolon, tok::identifier)) {

djasper wrote:
> No braces.
Done.



Comment at: lib/Format/TokenAnnotator.cpp:350
+const FormatToken *ParamToken = AttrTok->Next;
+while (ParamToken && ParamToken->isNot(tok::r_paren))
+  ParamToken = ParamToken->Next;

djasper wrote:
> Sorry that I have missed this before, I thought this was in a different file. 
> Can you try to avoid iterating trying to count or match parentheses inside 
> any of parseSquare/parseParen/parseAngle. We avoided that AFAICT for 
> everything else and I don't think it's necessary here. Especially as you are 
> not actually moving the token position forward, it's too easy to create a 
> quadratic algorithm here.
> 
> Also: Do you actually have a test case for the the parentheses case? This 
> thing could use a lot more comments...
Fixed. There is a test case for the parentheses case.

I definitely see from your other reviews that it's important to structure code 
the way you describe.

In this scenario, we technically have to deal with all of these 
(http://en.cppreference.com/w/cpp/language/attributes):

```
void f() {
  int y[3];
  int i [[cats::meow([[]])]]; // OK
  int i [[cats::meow(foo:bar)]]; // OK
  int i [[cats::meow(foo::bar)]]; // OK
}
```

I was able to fudge it and assume nobody will actually use attribute parameters 
containing colons in the arguments, which allowed me to avoid looking for 
closing parens. 



Comment at: lib/Format/TokenAnnotator.cpp:366
+  return AttrTok->Next;
+} else {
+  return nullptr;

djasper wrote:
> No braces for single statement ifs. Don't use "else" after "return".
Fixed.



Comment at: lib/Format/TokenAnnotator.cpp:396
+  while (CurrentToken != Cpp11AttributeSpecifierClosingRSquare) {
+if (CurrentToken->is(tok::colon)) {
+  CurrentToken->Type = TT_AttributeColon;

djasper wrote:
> No braces for single-statement ifs.
Fixed.



Comment at: lib/Format/TokenAnnotator.cpp:397
+if (CurrentToken->is(tok::colon)) {
+  CurrentToken->Type = TT_AttributeColon;
+}

djasper wrote:
> What happens if you don't assign this type here? Which formatting decision is 
> based on it?
Added a comment to explain:

  // Remember that this is a [[using ns: foo]] C++ attribute, so we 
don't   
  
  // add a space before the colon (unlike other colons).

  

There is a formatting test for this.



Comment at: unittests/Format/FormatTest.cpp:6064
+  verifyFormat("SomeType s [[unused]] (InitValue);");
+  verifyFormat("SomeType s [[gnu::unused]] (InitValue);");
+  verifyFormat("SomeType s [[using gnu: unused]] (InitValue);");

djasper wrote:
> If this is meant to contrast a TT_AttributeColon from a different colon, that 
> doesn't work. "::" is it's own token type coloncolon.
It's not, I just wanted to make sure we had a test with a namespace.


Repository:
  rC Clang

https://reviews.llvm.org/D43902



_

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton marked an inline comment as done.
benhamilton added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:210
 
-bool MightBeFunctionType = !Contexts[Contexts.size() - 2].IsExpression;
-bool ProbablyFunctionType = CurrentToken->isOneOf(tok::star, tok::amp);
+bool MightBeFunctionOrObjCBlockType =
+!Contexts[Contexts.size() - 2].IsExpression;

djasper wrote:
> I'd suggest to put a comment here saying that this is for both ObjC blocks 
> and Function types, because they look very similar in nature (maybe giving 
> examples) and then not actually rename the variables. To me, the long names 
> make the code harder to read.
> 
> But if you feel strongly the other way, I'd be ok with it.
Restored old names and added comment.



Repository:
  rC Clang

https://reviews.llvm.org/D43906



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

[PATCH] D43902: [clang-format] Don't detect C++11 attribute specifiers as ObjC

[Ben Hamilton via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC327284: [clang-format] Don't detect C++11 attribute 
specifiers as ObjC (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43902?vs=137696&id=138025#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43902

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -29,7 +29,9 @@
 #define LIST_TOKEN_TYPES   \
   TYPE(ArrayInitializerLSquare)\
   TYPE(ArraySubscriptLSquare)  \
+  TYPE(AttributeColon) \
   TYPE(AttributeParen) \
+  TYPE(AttributeSquare)\
   TYPE(BinaryOperator) \
   TYPE(BitFieldColon)  \
   TYPE(BlockComment)   \
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -328,13 +328,40 @@
 return false;
   }
 
+  bool isCpp11AttributeSpecifier(const FormatToken &Tok) {
+if (!Style.isCpp() || !Tok.startsSequence(tok::l_square, tok::l_square))
+  return false;
+const FormatToken *AttrTok = Tok.Next->Next;
+if (!AttrTok)
+  return false;
+// C++17 '[[using ns: foo, bar(baz, blech)]]'
+// We assume nobody will name an ObjC variable 'using'.
+if (AttrTok->startsSequence(tok::kw_using, tok::identifier, tok::colon))
+  return true;
+if (AttrTok->isNot(tok::identifier))
+  return false;
+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(:)]]
+  if (AttrTok->is(tok::colon) ||
+  AttrTok->startsSequence(tok::identifier, tok::identifier))
+return false;
+  if (AttrTok->is(tok::ellipsis))
+return true;
+  AttrTok = AttrTok->Next;
+}
+return AttrTok && AttrTok->startsSequence(tok::r_square, tok::r_square);
+  }
+
   bool parseSquare() {
 if (!CurrentToken)
   return false;
 
 // A '[' could be an index subscript (after an identifier or after
 // ')' or ']'), it could be the start of an Objective-C method
-// expression, or it could the start of an Objective-C array literal.
+// expression, it could the start of an Objective-C array literal,
+// or it could be a C++ attribute specifier [[foo::bar]].
 FormatToken *Left = CurrentToken->Previous;
 Left->ParentBracket = Contexts.back().ContextKind;
 FormatToken *Parent = Left->getPreviousNonComment();
@@ -347,8 +374,11 @@
 (Contexts.back().CanBeExpression || Contexts.back().IsExpression ||
  Contexts.back().InTemplateArgument);
 
+bool IsCpp11AttributeSpecifier = isCpp11AttributeSpecifier(*Left) ||
+ Contexts.back().InCpp11AttributeSpecifier;
+
 bool StartsObjCMethodExpr =
-!CppArrayTemplates && Style.isCpp() &&
+!CppArrayTemplates && Style.isCpp() && !IsCpp11AttributeSpecifier &&
 Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
 CurrentToken->isNot(tok::l_brace) &&
 (!Parent ||
@@ -365,6 +395,8 @@
 } else if (Left->is(TT_Unknown)) {
   if (StartsObjCMethodExpr) {
 Left->Type = TT_ObjCMethodExpr;
+  } else if (IsCpp11AttributeSpecifier) {
+Left->Type = TT_AttributeSquare;
   } else if (Style.Language == FormatStyle::LK_JavaScript && Parent &&
  Contexts.back().ContextKind == tok::l_brace &&
  Parent->isOneOf(tok::l_brace, tok::comma)) {
@@ -432,11 +464,14 @@
   Contexts.back().IsExpression = false;
 
 Contexts.back().ColonIsObjCMethodExpr = StartsObjCMethodExpr;
+Contexts.back().InCpp11AttributeSpecifier = IsCpp11AttributeSpecifier;
 
 while (CurrentToken) {
   if (CurrentToken->is(tok::r_square)) {
-if (CurrentToken->Next && CurrentToken->Next->is(tok::l_paren) &&
-Left->is(TT_ObjCMethodExpr)) {
+if (IsCpp11AttributeSpecifier)
+  CurrentToken->Type = TT_AttributeSquare;
+else if (CurrentToken->Next && CurrentToken->Next->is(tok::l_paren) &&
+

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

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

- Restore short functionn type variable names and add clarifying comment.


Repository:
  rC Clang

https://reviews.llvm.org/D43906

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
"  foo();\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12128,6 +12128,22 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^, Bar);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[kNumEntries])(char, float);"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -141,10 +141,7 @@
 Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
 bool StartsObjCMethodExpr = false;
-if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
-} else if (FormatToken *MaybeSel = Left->Previous) {
+if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
   if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous 
&&
   MaybeSel->Previous->is(tok::at)) {
@@ -210,8 +207,16 @@
   Left->Type = TT_ObjCMethodExpr;
 }
 
+// MightBeFunctionType and ProbablyFunctionType are used for
+// function pointer and reference types as well as Objective-C
+// block types:
+//
+// void (*FunctionPointer)(void);
+// void (&FunctionReference)(void);
+// void (^ObjCBlock)(void);
 bool MightBeFunctionType = !Contexts[Contexts.size() - 2].IsExpression;
-bool ProbablyFunctionType = CurrentToken->isOneOf(tok::star, tok::amp);
+bool ProbablyFunctionType =
+CurrentToken->isOneOf(tok::star, tok::amp, tok::caret);
 bool HasMultipleLines = false;
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
@@ -248,7 +253,8 @@
 if (MightBeFunctionType && ProbablyFunctionType && CurrentToken->Next 
&&
 (CurrentToken->Next->is(tok::l_paren) ||
  (CurrentToken->Next->is(tok::l_square) && 
Line.MustBeDeclaration)))
-  Left->Type = TT_FunctionTypeLParen;
+  Left->Type = Left->Next->is(tok::caret) ? TT_ObjCBlockLParen
+  : TT_FunctionTypeLParen;
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
 


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
"  foo();\n"
Index: unittests/Format/FormatTest.cpp

[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
benhamilton marked an inline comment as done.
Closed by commit rC327285: [clang-format] Improve detection of Objective-C 
block types (authored by benhamilton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43906?vs=138024&id=138026#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43906

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


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -141,10 +141,7 @@
 Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
 bool StartsObjCMethodExpr = false;
-if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
-} else if (FormatToken *MaybeSel = Left->Previous) {
+if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
   if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous 
&&
   MaybeSel->Previous->is(tok::at)) {
@@ -210,8 +207,16 @@
   Left->Type = TT_ObjCMethodExpr;
 }
 
+// MightBeFunctionType and ProbablyFunctionType are used for
+// function pointer and reference types as well as Objective-C
+// block types:
+//
+// void (*FunctionPointer)(void);
+// void (&FunctionReference)(void);
+// void (^ObjCBlock)(void);
 bool MightBeFunctionType = !Contexts[Contexts.size() - 2].IsExpression;
-bool ProbablyFunctionType = CurrentToken->isOneOf(tok::star, tok::amp);
+bool ProbablyFunctionType =
+CurrentToken->isOneOf(tok::star, tok::amp, tok::caret);
 bool HasMultipleLines = false;
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
@@ -248,7 +253,8 @@
 if (MightBeFunctionType && ProbablyFunctionType && CurrentToken->Next 
&&
 (CurrentToken->Next->is(tok::l_paren) ||
  (CurrentToken->Next->is(tok::l_square) && 
Line.MustBeDeclaration)))
-  Left->Type = TT_FunctionTypeLParen;
+  Left->Type = Left->Next->is(tok::caret) ? TT_ObjCBlockLParen
+  : TT_FunctionTypeLParen;
 Left->MatchingParen = CurrentToken;
 CurrentToken->MatchingParen = Left;
 
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12128,6 +12128,22 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "[[foo::bar, ...]]"));
 }
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^, Bar);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo)(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[10])(char, float);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "int(^foo[kNumEntries])(char, float);"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -845,6 +845,15 @@
   verifyFormat("@ /*foo*/ interface");
 }
 
+TEST_F(FormatTestObjC, ObjCBlockTypesAndVariables) {
+  verifyFormat("void DoStuffWithBlockType(int (^)(char));");
+  verifyFormat("int (^foo)(char, float);");
+  verifyFormat("int (^foo[10])(char, float);");
+  verifyFormat("int (^foo[kNumEntries])(char, float);");
+  verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
+  verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+}
+
 TEST_F(FormatTestObjC, ObjCSnippets) {
   verifyFormat("@autoreleasepool {\n"
"  foo();\n"


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -141,10 +141,7 @@
 Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
 bool StartsObjCMethodExpr = false;
-if (CurrentToken->is(tok::caret)) {
-  // (^ can start a block type.
-  Left->Type = TT_ObjCBlockLParen;
-} else if (FormatToken *MaybeSel = Left->Previous) {
+if (FormatToken *MaybeSel = Left->Previous) {
   // @selector( starts a selector.
   if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous &&
   MaybeSel->Previous->is(tok::

[PATCH] D25820: [Sema][Objective-C] Formatting warnings should see through Objective-C message sends

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

>@benhamilton We're planning to use the new attribute in 
>https://reviews.llvm.org/D27165 to address this issue. I'll get back to that 
>patch in the near future.

Great, thanks for the update.


Repository:
  rL LLVM

https://reviews.llvm.org/D25820



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

[PATCH] D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic

[Ben Hamilton via Phabricator via cfe-commits]

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

We received reports of the Objective-C style guesser getting a false
negative on header files like:

CGSize SizeOfThing(MyThing thing);

This adds more Core Graphics identifiers to the Objective-C style
guesser.

Test Plan: New tests added. Ran tests with:

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


Repository:
  rC Clang

https://reviews.llvm.org/D44632

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


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12096,6 +12096,11 @@
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "@interface 
Foo\n@end\n"));
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface 
Foo\n@end\n"));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "int DoStuff(CGRect 
rect);\n"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h",
+"#define MY_POINT_MAKE(x, y) CGPointMake((x), (y));\n"));
 }
 
 TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1449,6 +1449,19 @@
 // Keep this array sorted, since we are binary searching over it.
 static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
 "CGFloat",
+"CGPoint",
+"CGPointMake",
+"CGPointZero",
+"CGRect",
+"CGRectEdge",
+"CGRectInfinite",
+"CGRectMake",
+"CGRectNull",
+"CGRectZero",
+"CGSize",
+"CGSizeMake",
+"CGVector",
+"CGVectorMake",
 "NSAffineTransform",
 "NSArray",
 "NSAttributedString",
@@ -1497,6 +1510,8 @@
 "NSURLQueryItem",
 "NSUUID",
 "NSValue",
+"UIImage",
+"UIView",
 };
 
 for (auto &Line : AnnotatedLines) {


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12096,6 +12096,11 @@
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "@interface Foo\n@end\n"));
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n@end\n"));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "int DoStuff(CGRect rect);\n"));
+  EXPECT_EQ(
+  FormatStyle::LK_ObjC,
+  guessLanguage("foo.h",
+"#define MY_POINT_MAKE(x, y) CGPointMake((x), (y));\n"));
 }
 
 TEST_F(FormatTest, GuessLanguageWithCpp11AttributeSpecifiers) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1449,6 +1449,19 @@
 // Keep this array sorted, since we are binary searching over it.
 static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
 "CGFloat",
+"CGPoint",
+"CGPointMake",
+"CGPointZero",
+"CGRect",
+"CGRectEdge",
+"CGRectInfinite",
+"CGRectMake",
+"CGRectNull",
+"CGRectZero",
+"CGSize",
+"CGSizeMake",
+"CGVector",
+"CGVectorMake",
 "NSAffineTransform",
 "NSArray",
 "NSAttributedString",
@@ -1497,6 +1510,8 @@
 "NSURLQueryItem",
 "NSUUID",
 "NSValue",
+"UIImage",
+"UIView",
 };
 
 for (auto &Line : AnnotatedLines) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44634: [clang-format] Detect Objective-C for #import

[Ben Hamilton via Phabricator via cfe-commits]

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

Previously, the Objective-C heuristic failed to detect
headers which #imported Objective-C system framework(s) and declared C
functions on top them.

This extends the heuristic to look for statements of the form:

and check if "Foo" is one of the known Objective-C system frameworks.

Test Plan: New tests added. Ran tests with:

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


Repository:
  rC Clang

https://reviews.llvm.org/D44634

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12097,6 +12097,14 @@
   EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo", ""));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n@end\n"));
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "int DoStuff(CGRect rect);\n"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "#import \n"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+guessLanguage("foo.h", "#import \n"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "#import \n"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "#include \n"));
   EXPECT_EQ(
   FormatStyle::LK_ObjC,
   guessLanguage("foo.h",
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1513,6 +1513,79 @@
 "UIImage",
 "UIView",
 };
+// Keep this array sorted, since we are binary searching over it.
+static constexpr llvm::StringLiteral ObjCFrameworks[] = {
+"ARKit",
+"AVFoundation",
+"Accounts",
+"AddressBookUI",
+"AppKit",
+"AssetsLibrary",
+"AudioToolbox",
+"CloudKit",
+"Contacts",
+"ContactsUI",
+"CoreAudioKit",
+"CoreBluetooth",
+"CoreData",
+"CoreImage",
+"CoreLocation",
+"CoreMIDI",
+"CoreML",
+"CoreMotion",
+"CoreNFC",
+"CoreSpotlight",
+"CoreTelephony",
+"DeviceCheck",
+"EventKit",
+"ExternalAccessory",
+"FileProvider",
+"FileProviderUI",
+"Foundation",
+"GLKit",
+"GameController",
+"GameKit",
+"GameplayKit",
+"HealthKit",
+"HomeKit",
+"IOSurface",
+"IdentityLookup",
+"Intents",
+"IntentsUI",
+"JavaScriptCore",
+"MapKit",
+"MediaPlayer",
+"MessageUI",
+"Metal",
+"MetalKit",
+"MetalPerformanceShaders",
+"ModelIO",
+"MultipeerConnectivity",
+"NetworkExtension",
+"NewsstandKit",
+"NotificationCenter",
+"PDFKit",
+"PassKit",
+"Photos",
+"PhotosUI",
+"PushKit",
+"QuartzCore",
+"QuickLook",
+"ReplayKit",
+"SafariServices",
+"SceneKit",
+"Social",
+"Speech",
+"SpriteKit",
+"StoreKit",
+"UIKit",
+"UserNotifications",
+"VideoSubscriberAccount",
+"Vision",
+"WatchConnectivity",
+"WatchKit",
+"WebKit",
+};
 
 for (auto &Line : AnnotatedLines) {
   for (FormatToken *FormatTok = Line->First; FormatTok;
@@ -1534,6 +1607,18 @@
TT_ObjCDecl, TT_ObjCForIn, TT_ObjCMethodExpr,
TT_ObjCMethodSpecifier, TT_ObjCProperty)) {
   return true;
+} else if (Line->Type == LT_ImportStatement &&
+   !FormatTok->getPreviousNonComment() &&
+   // #import 
+   FormatTok->startsSequence(
+   tok::hash, tok::identifier, tok::less, tok::identifier,
+   tok::slash, tok::identifier, tok::period,
+   tok::identifier, tok::greater) &&
+   FormatTok->Next->TokenText == "import" &&
+   std::binary_search(std::begin(ObjCFrameworks),
+  std::end(ObjCFrameworks),
+  FormatTok->Next->Next->Next->TokenText)) {
+  return true;
 }
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

I wonder if this wouldn't be better as a clang-tidy check:

https://github.com/llvm-mirror/clang-tools-extra/tree/master/clang-tidy/objc


Repository:
  rC Clang

https://reviews.llvm.org/D44539



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

[PATCH] D44638: [clang-format] Fix ObjC selectors with multiple params passed to macro

[Ben Hamilton via Phabricator via cfe-commits]

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

Objective-C selectors can with arguments be in the form:

foo:
foo:bar:
foo:bar:baz:

These can be passed to a macro, like NS_SWIFT_NAME():

https://developer.apple.com/library/content/documentation/Swift/Conceptual/BuildingCocoaApps/MixandMatch.html

and must never have spaces inserted around the colons.

Previously, there was logic in TokenAnnotator's tok::colon parser to
handle the single-argument case, but it failed for the
multiple-argument cases.

This diff fixes the bug and adds more tests.

Test Plan: New tests added. Ran tests with:

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


Repository:
  rC Clang

https://reviews.llvm.org/D44638

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


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -618,6 +618,9 @@
   verifyFormat("for (id foo in [self getStuffFor:bla]) {\n"
"}");
   verifyFormat("[self a:MACRO(a, b:, c:)];");
+  verifyFormat("[self a:MACRO(a, b:c:, d:e:)];");
+  verifyFormat("[self a:MACRO(a, b:c:d:, e:f:g:)];");
+  verifyFormat("int XYMyFoo(int a, int b) NS_SWIFT_NAME(foo(self:scale:));");
   verifyFormat("[self a:(1 + 2) b:3];");
   verifyFormat("[self a:(Type)a b:3];");
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -701,7 +701,8 @@
 else
   Tok->Type = TT_InheritanceColon;
   } else if (Tok->Previous->is(tok::identifier) && Tok->Next &&
- Tok->Next->isOneOf(tok::r_paren, tok::comma)) {
+ (Tok->Next->isOneOf(tok::r_paren, tok::comma) ||
+  Tok->Next->startsSequence(tok::identifier, tok::colon))) {
 // This handles a special macro in ObjC code where selectors including
 // the colon are passed as macro arguments.
 Tok->Type = TT_ObjCMethodExpr;


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -618,6 +618,9 @@
   verifyFormat("for (id foo in [self getStuffFor:bla]) {\n"
"}");
   verifyFormat("[self a:MACRO(a, b:, c:)];");
+  verifyFormat("[self a:MACRO(a, b:c:, d:e:)];");
+  verifyFormat("[self a:MACRO(a, b:c:d:, e:f:g:)];");
+  verifyFormat("int XYMyFoo(int a, int b) NS_SWIFT_NAME(foo(self:scale:));");
   verifyFormat("[self a:(1 + 2) b:3];");
   verifyFormat("[self a:(Type)a b:3];");
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -701,7 +701,8 @@
 else
   Tok->Type = TT_InheritanceColon;
   } else if (Tok->Previous->is(tok::identifier) && Tok->Next &&
- Tok->Next->isOneOf(tok::r_paren, tok::comma)) {
+ (Tok->Next->isOneOf(tok::r_paren, tok::comma) ||
+  Tok->Next->startsSequence(tok::identifier, tok::colon))) {
 // This handles a special macro in ObjC code where selectors including
 // the colon are passed as macro arguments.
 Tok->Type = TT_ObjCMethodExpr;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits