[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 05:56:16 PDT 2022

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
+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

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list