kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
Thanks, LGTM! As you mentioned I believe `std::move` is common enough to
deserve the special casing.
================
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>.
----------------
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.)
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
