kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:393-395
+ const auto *FileEntry = SM.getFileEntryForID(FID);
+ for (const auto *Export : PI.getExporters(FileEntry, SM.getFileManager()))
+ return toURI(Export->tryGetRealPathName());
----------------
sorry i don't understand what this logic is trying to achieve.
we seem to be prefering "first" exporting header, over the headers that
directly provide the symbol. Also comment says it's done for objc, but there's
nothing limiting this logic to ObjC. any chance this is unintended?
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:791
+ include_cleaner::Macro{const_cast<IdentifierInfo *>(Name), DefLoc},
+ ASTCtx->getSourceManager(), &PI);
+ setSymbolProviders(S, Headers);
----------------
s/ASTCtx->getSourceManager()/SM
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:837
+ const Symbol &S,
+ const llvm::SmallVector<include_cleaner::Header> &Headers) {
+ if (Opts.CollectIncludePath &&
----------------
you can take this by value and `std::move` into the map
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:903
+ NewSym = *S;
+ if (!IncludeHeader.empty()) {
NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives});
----------------
this is using legacy mappings for non-objc symbols too
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:915
+ NewSym.IncludeHeaders.push_back(
+ {HeaderFileURIs->getIncludeHeader(FID), 1, Directives});
+ Symbols.insert(NewSym);
----------------
`getIncludeHeader` might return an empty string
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:928
+ case include_cleaner::Header::Kind::Verbatim:
+ SymbolIncludeSpelling[SID] = H.verbatim().str();
+ break;
----------------
you're iterating over multiple headers, and only keeping references for the
last one alive.
instead of storing these in the class state, you can have a
`llvm::DenseMap<Header, std::string> HeaderToSpelling;` inside this function
and later on just do:
```
for(const auto& H: It->second) {
auto [SpellingIt, Inserted] = HeaderToSpelling.try_emplace(H);
if(Inserted) {
switch (...) { /* Update *SpellingIt */ }
}
NewSym.IncludeHeaders.push_back(*SpellingIt, 1, Directives);
```
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:934-936
+ SymbolIncludeSpelling[SID] = HeaderFileURIs->getIncludeHeader(
+ ASTCtx->getSourceManager().getOrCreateFileID(
+ H.physical(), SrcMgr::CharacteristicKind::C_User));
----------------
we actually only want to use `HeaderFileURIs->toURI` here and not the
`getIncludeHeader`, because:
- We want to make use of mapping logic in include-cleaner solely, and we
already have all that information in Providers mapping.
- The extra logic we want to apply for ObjC symbols are already applied before
reaching here.
the way we create a FileID here is a little bit iffy, by using `toURI`,
hopefully we can avoid that conversion.
the only remaining issue is, we actually still want to use system header
mappings inside `CanonicalIncludes` until we have better support for them in
include-cleaner library itself. So I think we should still call
`Includes->mapHeader(H.physical())` before using `toURI`.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:131
+ }
+ void setPreprocessor(Preprocessor &PP) {
+ this->PP = &PP;
----------------
unfortunately this alternative is called after we've parsed the file, from
`clangd/index/FileIndex.cpp`, `indexSymbols`. hence attaching `PI` to
`PPCallbacks` won't have any effect.
This is working unintentionally because we're still populating
`CanonicalIncludes` with legacy IWYUPragmaHandler in clangd, and using that to
perform private -> public mappings.
We should instead propagate the `PragmaIncludes` we have in `ParsedAST` and in
preamble builds here. You should be able to test the new behaviour
`clang-tools-extra/clangd/unittests/FileIndexTests.cpp` to make sure pragma
handling through dynamic indexing code paths keep working as expected using an
`export` pragma.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152900/new/
https://reviews.llvm.org/D152900
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
