[PATCH] D152900: [clangd] Update symbol collector to use include-cleaner.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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