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