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

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a subscriber: krasimir.
benhamilton added a comment.

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



================
Comment at: lib/Format/Format.cpp:1450
     // Keep this array sorted, since we are binary searching over it.
     static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
         "CGFloat",
----------------
jolesiak wrote:
> djasper wrote:
> > 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?
> It was a hash set initially: D42135
> Changed in: D42189
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/


================
Comment at: unittests/Format/FormatTest.cpp:12099
   EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo", "@interface Foo\n at end\n"));
+  EXPECT_EQ(FormatStyle::LK_ObjC, guessLanguage("foo.h", "int DoStuff(CGRect rect);\n"));
+  EXPECT_EQ(
----------------
djasper wrote:
> jolesiak wrote:
> > 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.
> Agreed. Please format this file with clang-format.
Oops! Fixed. (Should we put in an `arc lint` check that code is correctly `clang-format`ted?)


Repository:
  rC Clang

https://reviews.llvm.org/D44632