benhamilton marked 4 inline comments as done.
benhamilton 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",
----------------
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.
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
