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

[benhamilton]

Summary

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

[jolesiak]

unittests/Format/FormatTest.cpp

12099 ↗

(On Diff #138944)

I know that it's violated in several places in this file (even in two of the three lines above), but I feel like we should keep 80 char limit for column width.

[djasper]

lib/Format/Format.cpp

1450 ↗

(On Diff #138944)

I have concerns about this growing lists of things. Specifically:

• Keeping it sorted is a maintenance concern.
• Doing binary search for basically every identifier in a header seems an unnecessary waste.

Could we just create a hash set of these?

unittests/Format/FormatTest.cpp

12099 ↗

(On Diff #138944)

Agreed. Please format this file with clang-format.

[jolesiak]

lib/Format/Format.cpp

1450 ↗

(On Diff #138944)

It was a hash set initially: D42135
Changed in: D42189

[benhamilton]

clang-format

[benhamilton]

@krasimir, can you answer @djasper's question about hash set vs. binary search?

lib/Format/Format.cpp

1450 ↗

(On Diff #138944)

Happy to do whatever folks recommend; I assume @krasimir's concern in D42189 was the startup cost of creating the (read-only) hash set.

We can automate keeping this sorted with an arc lint check, they're quite easy to write:

https://secure.phabricator.com/book/phabricator/article/arcanist_lint/

unittests/Format/FormatTest.cpp

12099 ↗

(On Diff #138944)

Oops! Fixed. (Should we put in an arc lint check that code is correctly clang-formatted?)

[djasper]

lib/Format/Format.cpp

1450 ↗

(On Diff #138944)

Krasimir clarified this to me offline. I have no concerns staying with binary search here and for this patch so long as someone builds in an assert that warns us when the strings here are not in the right order at some point.

[benhamilton]

Add assert(std::is_sorted(...)). (We can't static_assert on is_sorted until C++2x.)

[benhamilton]

lib/Format/Format.cpp

1450 ↗

(On Diff #138944)

Good call, added assert(std::is_sorted(...)).

I tried static_assert, but std::is_sorted() is not constexpr until c++2x.

[jolesiak]

lib/Format/Format.cpp

1450 ↗

(On Diff #138944)

Checking this type of constraints in arc lint sounds rather weird. I like this assert as testing private methods is painful.

unittests/Format/FormatTest.cpp

12099 ↗

(On Diff #138944)

I don't know what is convention here, but to me using clang-format to format clang-format code sounds good. It's a little bit surprising that it's not the case.

[krasimir]

lib/Format/Format.cpp

1450 ↗

(On Diff #138944)

I now think a hash set here is better. Sent https://reviews.llvm.org/D44695 to replace the array with that.

Sorry for wasting everybody's time.

[jolesiak]

lib/Format/Format.cpp

1450 ↗

(On Diff #138944)

Then assertion is no longer needed.

[djasper]

Looks good.

[benhamilton]

Remove assert

cfe/trunk/lib/Format/Format.cpp

public:
   bool isObjC() { return IsObjC; }
 
 private:
   static bool guessIsObjC(const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
                           const AdditionalKeywords &Keywords) {
     // 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",
         "NSBundle",
         "NSCache",
         "NSCalendar",
         "NSCharacterSet",
         "NSCountedSet",
static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
         "NSString",
         "NSTimeZone",
         "NSUInteger",
         "NSURL",
         "NSURLComponents",
         "NSURLQueryItem",
         "NSUUID",
         "NSValue",
+        "UIImage",
+        "UIView",
     };
 
     for (auto &Line : AnnotatedLines) {
       for (FormatToken *FormatTok = Line->First; FormatTok;
            FormatTok = FormatTok->Next) {
         if ((FormatTok->Previous && FormatTok->Previous->is(tok::at) &&
              (FormatTok->isObjCAtKeyword(tok::objc_interface) ||
               FormatTok->isObjCAtKeyword(tok::objc_implementation) ||

cfe/trunk/unittests/Format/FormatTest.cpp

 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"));
+  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) {
   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\")]];"));