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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:38
 public:
-  /// Adds a string-to-string mapping from \p Path to \p CanonicalPath.
-  void addMapping(llvm::StringRef Path, llvm::StringRef CanonicalPath);
+  /// Adds a string-to-string mapping from \p ID to \p CanonicalPath.
+  void addMapping(const FileEntry *Header, llvm::StringRef CanonicalPath);
----------------
nit: no longer a string-to-string mapping


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:45
   /// Returns the overridden include for for files in \p Header, or "".
-  llvm::StringRef mapHeader(llvm::StringRef Header) const;
+  llvm::StringRef mapHeader(const FileEntry *Header) const;
 
----------------
prefer FileEntryRef over FileEntry*, so the name we're looking up is controlled by the caller rather than an ~arbitrary choice in case of symlinks


================
Comment at: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp:1
 //===-- CanonicalIncludesTests.cpp - --------------------------------------===//
 //
----------------
Can you add a test for the new functionality? that the mapping doesn't depend on the name

Use the hard-link function in InMemoryFS to create multiple names with one UID


================
Comment at: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp:57
+  auto InMemFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  llvm::IntrusiveRefCntPtr<FileManager> Files(
+      new FileManager(FileSystemOptions(), InMemFS));
----------------
why the pointer? can't this just be `FileManager Files(...)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123031