kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:841-847 auto [It, Inserted] = SymbolProviders.try_emplace(S.ID); if (Inserted) { auto Headers = include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes); if (!Headers.empty()) It->second = Headers.front(); } ---------------- can you also get rid of this piece, and `SymbolProviders` map ? ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:873 llvm::DenseMap<FileID, bool> FileToContainsImportsOrObjC; llvm::DenseMap<include_cleaner::Header, std::string> HeaderSpelling; // Fill in IncludeHeaders. ---------------- we can drop this too ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:903-927 + // mapping inside the include-cleaner library again. + llvm::StringRef IncludeHeader; + if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name)) + if (auto Header = StdSym->header()) + IncludeHeader = Header->name(); + if (S->Scope == "std::" && S->Name == "move") { + IncludeHeader = "<utility>"; ---------------- rather than duplicating some of the checks, can we rewrite this block as: ``` llvm::StringRef IncludeHeader = getStdlibHeader(*S, ASTCtx->getLangOpts()); if (IncludeHeader.empty()) IncludeHeader = HeaderFileURIs->getIncludeHeader(FID); if (!IncludeHeader.empty()) { auto NewSym = *S; NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives}); Symbols.insert(NewSym); } ``` ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:905 + llvm::StringRef IncludeHeader; + if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name)) + if (auto Header = StdSym->header()) ---------------- let's pass in the language options from ASTCtx into here ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:908-912 + if (S->Scope == "std::" && S->Name == "move") { + IncludeHeader = "<utility>"; + if (S->Signature.contains(',')) + IncludeHeader = "<algorithm>"; + } ---------------- i think the old order of, first dealing with `std::move` and then using `tooling::stdlib` is less error-prone and the order in which we apply the mapping in include-cleaner, so that would be a more compatible change going forward. ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:929-964 + // FIXME: use providers from include-cleaner library once it's polished + // for Objective-C. + continue; + + // FIXME: Re-enable for C++ code. The code below uses include-cleaner + // library but is currently unreachable due to regression. assert(Directives == Symbol::Include); ---------------- let's drop this block to not create confusion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156403/new/ https://reviews.llvm.org/D156403 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits