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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for comments!



================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:40
   /// Adds a file-to-string mapping from \p ID to \p CanonicalPath.
   void addMapping(FileEntryRef Header, llvm::StringRef CanonicalPath);
 
----------------
kadircet wrote:
> AFAICT, only users of this endpoint were in `IWYUCommentHandler` (and some tests). we can also get rid of this one now.
> 
> More importantly now this turns into a mapper used only by symbolcollector, that can be cheaply created at use time (we just copy around a pointer every time we want to create it). so you can drop `CanonicalIncludes` from all of the interfaces, including `SymbolCollector::Options` and create one on demand in `HeaderFileURICache`. all we need is langopts, and it's available through preprocessor (not necessarily on construction, but at the time when we want to do the mappings).
> 
> As you're already touching all of the interfaces that propagate `CanonicalIncludes` around, hopefully this change should only make things simpler (and you already need to touch them when you rebase), but if that feels like too much churn in this patch feel free to do that in a follow up as well.
> ... and create one on demand in HeaderFileURICache. all we need is langopts, and it's available through preprocessor (not necessarily on construction, but at the time when we want to do the mappings

This time sounds a little late, since we don't want to rebuild the system header mapping every time we request a mapping for a particular header. 
Cache construction time doesn't work, since it happens before the preprocessor is set in the symbol collector (and we need the preprocessor to retrieve language options).
So far the safe place I've found is right after the preprocessor is set in the symbol collector. Another option is to collect the system header options in the finish() method, but I can't see why we would need to delay until then. 


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:250
+  // of whether it's an otherwise-good header (header guards etc).
+  llvm::StringRef mapCanonical(FileID FID) {
+    if (SysHeaderMapping) {
----------------
kadircet wrote:
> let's change the interface here to take in a `FileEntry` instead.
I believe it has to be `FileEntryRef` instead. This is what the `CanonicalIncludes::mapHeader` expects, and this is also seems right (somewhere in the Clang docs I've read "everything that needs a name should use `FileEntryRef`", and name is exactly what `CanonicalIncludes::mapHeader` needs). 

For the case of a physical header (i.e., `FileEntry` instance) I've switched to using `getLastRef` now. There's also another API `FileManager::getFileRef` where I could try passing the result of `tryGetRealPathName`. I am not sure I have enough information to judge if any of these options is distinctly better.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:412-413
+
+    if (auto Verbatim = PI->getPublic(FileEntry); !Verbatim.empty())
+      return Verbatim;
+
----------------
kadircet wrote:
> let's move this logic into `mapCanonical` too.
This would deliver wrong results for C++. We would basically get every IWYU-public header twice: once through the `verbatim` branch of the include_cleaner results, and then once again when mapping the physical private file to a canonical version (e.g., for system headers).


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:849
+      shouldCollectIncludePath(S.SymInfo.Kind) != Symbol::Invalid)
+    SymbolProviders[S.ID] = std::move(Headers);
+}
----------------
kadircet wrote:
> because you're taking in `Headers` as `const`, this move is not actually moving.
Ok, this is not relevant anymore, but I am not sure I understand why. Am I not allowed to say that I don't need this variable/memory anymore, even though it's `const`?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:877
   // We delay this until end of TU so header guards are all resolved.
   for (const auto &[SID, FID] : IncludeFiles) {
     if (const Symbol *S = Symbols.find(SID)) {
----------------
kadircet wrote:
> let's iterate over `SymbolProviders` instead, as `IncludeFiles` is a legacy struct now.
Not sure I'm following. Iterating over `SymbolProviders` means retrieving `include_cleaner::Header`s. How would I handle the entire Obj-C part then, without the FID of the include header? 


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:930
+                auto Canonical =
+                    HeaderFileURIs->mapCanonical(SM.getOrCreateFileID(
+                        H.physical(), SrcMgr::CharacteristicKind::C_User));
----------------
kadircet wrote:
> no need for the `getOrCreateFileID` after you change parameter for `mapCanonical` to be a file entry.
Still need the `getLastRef` or `FileManager::getFileRef`, if I understand correctly.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:175
   SymbolSlab::Builder Symbols;
-  // File IDs for Symbol.IncludeHeaders.
-  // The final spelling is calculated in finish().
+  // File IDs to distinguish imports from includes.
   llvm::DenseMap<SymbolID, FileID> IncludeFiles;
----------------
kadircet wrote:
> we seem to be populating this map for all kinds of symbols, as long as `shouldCollectIncludePath` returns non-invalid. what's up with distinguishing `imports from includes`?
Yeah that's right. I've been trying to express that (after populating this map) we are not using this `FID` to compute the IncludeHeaders anymore (only for the ObjC case), but rather use it to determine if the `Directives` should contain `Symbol::Import` (ie., if this FID contains Obj-C constructs/imports).

I'll try to be a bit more verbose and precise. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152900/new/

https://reviews.llvm.org/D152900