[PATCH] D123031: [clangd] Use stable keys for CanonicalIncludes mappings

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Just nits



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:359
         if (Entry) {
-          auto PublicHeader = CanonIncludes.mapHeader(Entry->getName());
+          auto PublicHeader = CanonIncludes.mapHeader(Entry->getLastRef());
           if (!PublicHeader.empty()) {
----------------
getLastRef().getName() is just the same as Entry->getName().
We should be using SM.getFileEntryRefForID instead.

(Which is actually still not preserving the FERef under the hood, but will soon...)


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:40
+  /// Adds a file-to-string mapping from \p ID to \p CanonicalPath.
+  void addMapping(const FileEntryRef Header, llvm::StringRef CanonicalPath);
 
----------------
no const, FileEntryRef is passed by value (and below)


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:384
     if (Includes) {
-      llvm::StringRef Canonical = Includes->mapHeader(Filename);
+      llvm::StringRef Canonical = Includes->mapHeader(FE->getLastRef());
       if (!Canonical.empty()) {
----------------
again, avoid getLastRef


================
Comment at: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp:26
+FileEntryRef
+addFile(llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> &FS,
+        FileManager &FM, llvm::StringRef Filename) {
----------------
no need to take ownership, just take InMemoryFileSystem&?


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1593
+  CanonicalIncludes Includes;
+  Includes.addMapping((*File)->getLastRef(), "<canonical>");
+  CollectorOpts.CollectIncludePath = true;
----------------
avoid getLastRef


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123031