[PATCH] D53322: [clangd] Collect refs from headers.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks reasonable. +80% to index size is a shame, but not disastrous, and we have ways to shave it.

Can you check:

- how much we're increasing the in-memory size (Dex)
- that we're not outputting duplicate refs when preambles overlap between TUs



================
Comment at: clangd/index/IndexAction.h:24
 //   - references are always counted
-//   - main-file refs are collected (if RefsCallback is non-null)
+//   - refs in main file and #included headers are collected (if RefsCallback
+//     is non-null)
----------------
just "all references are collected"?


================
Comment at: clangd/index/SymbolCollector.cpp:480
 
+  llvm::DenseMap<clang::FileID, std::string> URICache;
   if (auto MainFileURI = toURI(SM, MainFileEntry->getName(), Opts)) {
----------------
maybe pull out
```auto GetURI = [&](clang::FileID FID) -> Optional<StringRef> {
  ... look up or populate cache entry ...
}```

then both the outer if and inner lookups can be written as:
`if (auto MainFileURI = GetURI(SM.getMainFileID())) {`
etc


================
Comment at: clangd/index/SymbolCollector.cpp:491
+              auto FileURI = toURI(SM, FileEntry->getName(), Opts);
+              if (!FileURI) {
+                log("Failed to create URI for file: {0}\n", FileEntry);
----------------
I think you probably also want to cache this case - using an empty string is fine


================
Comment at: clangd/index/SymbolCollector.h:62
+    /// otherwise, refs from headers included by main file will be collected.
+    /// This flag is only available when RefFilter is set.
+    bool RefMainFileOnly = true;
----------------
available -> meaningful


================
Comment at: clangd/index/SymbolCollector.h:63
+    /// This flag is only available when RefFilter is set.
+    bool RefMainFileOnly = true;
     // Every symbol collected will be stamped with this origin.
----------------
having "true" mean **don't** do something is a bit confusing.

What about `bool RefsInHeaders = false`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53322