[PATCH] D63931: [clangd] Make PreambleStatusCache handle filenames more carefully

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL364740: [clangd] Make PreambleStatusCache handle filenames more carefully (authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63931

Files:
  clang-tools-extra/trunk/clangd/FS.cpp
  clang-tools-extra/trunk/clangd/unittests/FSTests.cpp


Index: clang-tools-extra/trunk/clangd/unittests/FSTests.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/unittests/FSTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/FSTests.cpp
@@ -34,7 +34,7 @@
   // Main file is not cached.
   EXPECT_FALSE(StatCache.lookup(testPath("main")).hasValue());
 
-  llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(0, 0),
+  llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(123, 456),
                       std::chrono::system_clock::now(), 0, 0, 1024,
                       llvm::sys::fs::file_type::regular_file,
                       llvm::sys::fs::all_all);
@@ -42,7 +42,15 @@
   auto ConsumeFS = StatCache.getConsumingFS(FS);
   auto Cached = ConsumeFS->status(testPath("fake"));
   EXPECT_TRUE(Cached);
-  EXPECT_EQ(Cached->getName(), S.getName());
+  EXPECT_EQ(Cached->getName(), testPath("fake"));
+  EXPECT_EQ(Cached->getUniqueID(), S.getUniqueID());
+
+  // fake and temp/../fake should hit the same cache entry.
+  // However, the Status returned reflects the actual path requested.
+  auto CachedDotDot = ConsumeFS->status(testPath("temp/../fake"));
+  EXPECT_TRUE(CachedDotDot);
+  EXPECT_EQ(CachedDotDot->getName(), testPath("temp/../fake"));
+  EXPECT_EQ(CachedDotDot->getUniqueID(), S.getUniqueID());
 }
 
 } // namespace
Index: clang-tools-extra/trunk/clangd/FS.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/FS.cpp
+++ clang-tools-extra/trunk/clangd/FS.cpp
@@ -10,20 +10,25 @@
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/None.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
 
 namespace clang {
 namespace clangd {
 
-PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath)
-    : MainFilePath(MainFilePath) {
+PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath){
   assert(llvm::sys::path::is_absolute(MainFilePath));
+  llvm::SmallString<256> MainFileCanonical(MainFilePath);
+  llvm::sys::path::remove_dots(MainFileCanonical, /*remove_dot_dot=*/true);
+  this->MainFilePath = MainFileCanonical.str();
 }
 
 void PreambleFileStatusCache::update(const llvm::vfs::FileSystem &FS,
                                      llvm::vfs::Status S) {
+  // Canonicalize path for later lookup, which is usually by absolute path.
   llvm::SmallString<32> PathStore(S.getName());
   if (FS.makeAbsolute(PathStore))
     return;
+  llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true);
   // Do not cache status for the main file.
   if (PathStore == MainFilePath)
     return;
@@ -33,9 +38,15 @@
 
 llvm::Optional<llvm::vfs::Status>
 PreambleFileStatusCache::lookup(llvm::StringRef File) const {
-  auto I = StatCache.find(File);
+  // Canonicalize to match the cached form.
+  // Lookup tends to be first by absolute path, so no need to make absolute.
+  llvm::SmallString<256> PathLookup(File);
+  llvm::sys::path::remove_dots(PathLookup, /*remove_dot_dot=*/true);
+
+  auto I = StatCache.find(PathLookup);
   if (I != StatCache.end())
-    return I->getValue();
+    // Returned Status name should always match the requested File.
+    return llvm::vfs::Status::copyWithNewName(I->getValue(), File);
   return None;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D63931.207253.patch
Type: text/x-patch
Size: 3293 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190701/8a367b42/attachment.bin>