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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.


================
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:
> 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.
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.


Repository:
  rC Clang

https://reviews.llvm.org/D44632