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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
