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

[Jacek Olesiak via Phabricator via cfe-commits]

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


================
Comment at: lib/Format/Format.cpp:1450
     // Keep this array sorted, since we are binary searching over it.
     static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
         "CGFloat",
----------------
benhamilton wrote:
> djasper wrote:
> > benhamilton wrote:
> > > 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/
> > 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.
> Good call, added `assert(std::is_sorted(...))`.
> 
> I tried `static_assert`, but `std::is_sorted()` is not `constexpr` until c++2x.
Checking this type of constraints in `arc lint` sounds rather weird. I like this assert as testing private methods is painful.


================
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(
----------------
benhamilton wrote:
> 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?)
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.


Repository:
  rC Clang

https://reviews.llvm.org/D44632