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@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
