Page MenuHomePhabricator

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

Authored by benhamilton on Mar 19 2018, 8:44 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

benhamilton created this revision.Mar 19 2018, 8:44 AM
jolesiak requested changes to this revision.Mar 20 2018, 1:59 AM
jolesiak added inline comments.
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.

This revision now requires changes to proceed.Mar 20 2018, 1:59 AM
djasper added inline comments.Mar 20 2018, 2:40 AM
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 added inline comments.Mar 20 2018, 3:06 AM
lib/Format/Format.cpp
1450 ↗(On Diff #138944)

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

benhamilton marked 2 inline comments as done.

clang-format

@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 added inline comments.Mar 20 2018, 8:34 AM
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.

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

benhamilton marked 4 inline comments as done.Mar 20 2018, 8:56 AM
benhamilton added inline comments.
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 accepted this revision.Mar 20 2018, 9:00 AM
jolesiak added inline comments.
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.

This revision is now accepted and ready to land.Mar 20 2018, 9:00 AM
krasimir added inline comments.Mar 20 2018, 11:34 AM
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 added inline comments.Mar 21 2018, 4:44 AM
lib/Format/Format.cpp
1450 ↗(On Diff #138944)

Then assertion is no longer needed.

djasper accepted this revision.Mar 21 2018, 8:06 AM

Looks good.

benhamilton marked an inline comment as done.

Remove assert

This revision was automatically updated to reflect the committed changes.