[PATCH] D156403: [clangd] Revert the symbol collector behavior to old pre-include-cleaner-library behavior due to a regression.
Kadir Cetinkaya via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 27 01:57:14 PDT 2023
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
More information about the cfe-commits
mailing list