sammccall marked an inline comment as done.
sammccall added a comment.
In D88885#2314596 <https://reviews.llvm.org/D88885#2314596>, @kadircet wrote:
> Thanks, LGTM! As you mentioned I believe `std::move` is common enough to
> deserve the special casing.
Common enough and also literally the only case AFAIK (hopefully the committee
is friendly enough not to add more in future).
That combination pushes me towards preferring this hack at least for now...
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:563
if (auto Header = getIncludeHeader(QName, Entry.second)) {
+ // Hack: there are two std::move() overloads from different headers.
+ // CanonicalIncludes returns the common one-arg one from <utility>.
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > i think this should live inside `CanonicalIncludes`. What about changing
> > > `mapHeader` to take in an `llvm::Optional<size_t> NumParams` or
> > > `llvm::Optional<llvm::StringRef> Signature` ?
> > >
> > > That way we can actually get rid of the ambiguity caused by all of the
> > > overloads. I am not sure if number of parameters is always going to be
> > > enough tho so `Signature` might be a safer bet with some canonicalization
> > > so that we can match the version in cppreference.
> > >
> > > (I would prefer the solution above, but I am also fine with moving this
> > > into `SymbolCollector::getIncludeHeader` with a FIXME.)
> > That's kind of the point of this patch, I think this one special case isn't
> > worth making that library more complicated.
> >
> > Would also be happy with just always suggesting <utility>, or never
> > suggesting anything, though.
> Makes sense, I was suggesting `SymbolCollector::getIncludeHeader` rather than
> `SymbolCollector::finish` to keep the logic in here less complicated. But I
> am fine if you don't want to make changes to the singature. (It is
> unfortunate that `getIncludeHeader` is a private member instead of a free
> helper in here anyways.)
Oh right, of course. Done.
Yeah it's unfortunate, it's because isSelfContainedHeader is an expensive check
that we have to cache, so there's a bunch of state it needs access to.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88885/new/
https://reviews.llvm.org/D88885
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
