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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 5 02:58:36 PDT 2022

sammccall added inline comments.

Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:38
-  /// 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(...)`

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list