[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

[Sam McCall via Phabricator via cfe-commits]

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