kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:840 - if (S->Scope == "std::" && S->Name == "move") { - if (!S->Signature.contains(',')) - return "<utility>"; - return "<algorithm>"; - } - - if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name, Lang)) - if (auto Header = StdSym->header()) - return Header->name(); - return ""; + auto [It, Inserted] = SymbolProviders.try_emplace(S.ID); + auto Headers = ---------------- can you add a comment here saying, `We update providers for a symbol with each occurence, as SymbolCollector might run while parsing, rather than at the end of a translation unit. Hence we see more and more redecls over time.` ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:912 + if (Directives & Symbol::Import) { + if (auto IncludeHeader = HeaderFileURIs->getIncludeHeader(FID); + !IncludeHeader.empty()) { ---------------- we should keep the `getStdHeaders` logic for objc ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:934 + if (auto Canonical = + HeaderFileURIs->mapCanonical(H.physical()->getName()); + !Canonical.empty()) ---------------- can you add a `// FIXME: Get rid of this once include-cleaner has support for system headers.` to this branch ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:937 + SpellingIt->second = Canonical; + else if (tooling::isSelfContainedHeader(H.physical(), SM, + PP->getHeaderSearchInfo())) ---------------- again a comment saying `For physical files, prefer URIs as spellings might change depending on the translation unit.` ================ Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:127 + if (FD->getNumParams() == 3 || FD->getNumParams() == 4) // move(InputIt first, InputIt last, OutputIt dest); return tooling::stdlib::Header::named("<algorithm>"); ---------------- can you also add comment `move(ExecutionPolicy&& policy, ForwardIt1 first, ForwardIt1 last, ForwardIt2 d_first );` and move this change into a separate patch with a test case in clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp? ================ Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdSpecialSymbolMap.inc:365 +// Remove them when the cause(s) are identified. +SYMBOL(div, std::, <cstdlib>) +SYMBOL(abort, std::, <cstdlib>) ---------------- can you move this into a separate patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156659/new/ https://reviews.llvm.org/D156659 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits