[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

The `IsValid = true` line coincidentally fixes a tangentially related bug -- 
see https://reviews.llvm.org/D20338 (in which I tried to introduce the same fix 
almost a year earlier, but nobody accepted the review). I guess I have to 
maintain the test for that case out of tree, though I could try to upstream it 
again (however, given that it's now passing with your change, I don't have much 
hope of it being accepted).


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Erik Verbruggen via Phabricator via cfe-commits]

erikjv closed this revision.
erikjv added a comment.

Committed as r298905.


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer accepted this revision.
bkramer added a comment.
This revision is now accepted and ready to land.

lg


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Erik Verbruggen via Phabricator via cfe-commits]

erikjv updated this revision to Diff 93221.
erikjv added a comment.

Added a test for the specific scenario, and added asserts for validity of UFEs 
returned by getVirtualFile.


https://reviews.llvm.org/D27810

Files:
  lib/Basic/FileManager.cpp
  unittests/Basic/FileManagerTest.cpp

Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -12,6 +12,7 @@
 #include "clang/Basic/FileSystemStatCache.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
@@ -29,6 +30,12 @@
   llvm::StringMap StatCalls;
 
   void InjectFileOrDirectory(const char *Path, ino_t INode, bool IsFile) {
+#ifndef LLVM_ON_WIN32
+SmallString<128> NormalizedPath(Path);
+llvm::sys::path::native(NormalizedPath);
+Path = NormalizedPath.c_str();
+#endif
+
 FileData Data;
 Data.Name = Path;
 Data.Size = 0;
@@ -55,6 +62,12 @@
   LookupResult getStat(StringRef Path, FileData , bool isFile,
std::unique_ptr *F,
vfs::FileSystem ) override {
+#ifndef LLVM_ON_WIN32
+SmallString<128> NormalizedPath(Path);
+llvm::sys::path::native(NormalizedPath);
+Path = NormalizedPath.c_str();
+#endif
+
 if (StatCalls.count(Path) != 0) {
   Data = StatCalls[Path];
   return CacheExists;
@@ -140,6 +153,7 @@
 
   const FileEntry *file = manager.getFile("/tmp/test");
   ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
   EXPECT_EQ("/tmp/test", file->getName());
 
   const DirectoryEntry *dir = file->getDir();
@@ -164,6 +178,7 @@
   manager.getVirtualFile("virtual/dir/bar.h", 100, 0);
   const FileEntry *file = manager.getFile("virtual/dir/bar.h");
   ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
   EXPECT_EQ("virtual/dir/bar.h", file->getName());
 
   const DirectoryEntry *dir = file->getDir();
@@ -185,7 +200,9 @@
   const FileEntry *fileFoo = manager.getFile("foo.cpp");
   const FileEntry *fileBar = manager.getFile("bar.cpp");
   ASSERT_TRUE(fileFoo != nullptr);
+  ASSERT_TRUE(fileFoo->isValid());
   ASSERT_TRUE(fileBar != nullptr);
+  ASSERT_TRUE(fileBar->isValid());
   EXPECT_NE(fileFoo, fileBar);
 }
 
@@ -231,8 +248,8 @@
   statCache->InjectFile("abc/bar.cpp", 42);
   manager.addStatCache(std::move(statCache));
 
-  manager.getVirtualFile("abc/foo.cpp", 100, 0);
-  manager.getVirtualFile("abc/bar.cpp", 200, 0);
+  ASSERT_TRUE(manager.getVirtualFile("abc/foo.cpp", 100, 0)->isValid());
+  ASSERT_TRUE(manager.getVirtualFile("abc/bar.cpp", 200, 0)->isValid());
 
   EXPECT_EQ(manager.getFile("abc/foo.cpp"), manager.getFile("abc/bar.cpp"));
 }
@@ -246,6 +263,37 @@
   manager.removeStatCache(statCache);
 }
 
+// getFile() Should return the same entry as getVirtualFile if the file actually
+// is a virtual file, even if the name is not exactly the same (but is after
+// normalisation done by the file system, like on Windows). This can be checked
+// here by checkng the size.
+TEST_F(FileManagerTest, getVirtualFileWithDifferentName) {
+  // Inject fake files into the file system.
+  auto statCache = llvm::make_unique();
+  statCache->InjectDirectory("c:\\tmp", 42);
+  statCache->InjectFile("c:\\tmp\\test", 43);
+
+  manager.addStatCache(std::move(statCache));
+
+  // Inject the virtual file:
+  const FileEntry *file1 = manager.getVirtualFile("c:\\tmp\\test", 123, 1);
+  ASSERT_TRUE(file1 != nullptr);
+  ASSERT_TRUE(file1->isValid());
+  EXPECT_EQ(43U, file1->getUniqueID().getFile());
+  EXPECT_EQ(123, file1->getSize());
+
+  // Lookup the virtual file with a different name:
+  const FileEntry *file2 = manager.getFile("c:/tmp/test", 100, 1);
+  ASSERT_TRUE(file2 != nullptr);
+  ASSERT_TRUE(file2->isValid());
+  // Check that it's the same UFE:
+  EXPECT_EQ(file1, file2);
+  EXPECT_EQ(43U, file2->getUniqueID().getFile());
+  // Check that the contents of the UFE are not overwritten by the entry in the
+  // filesystem:
+  EXPECT_EQ(123, file2->getSize());
+}
+
 #endif  // !LLVM_ON_WIN32
 
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -386,6 +386,7 @@
   UFE->ModTime = ModificationTime;
   UFE->Dir = DirInfo;
   UFE->UID = NextFileUID++;
+  UFE->IsValid = true;
   UFE->File.reset();
   return UFE;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Please attach a testcase!


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Erik Verbruggen via Phabricator via cfe-commits]

erikjv added a comment.

@klimek no, it's a 1 line fix. The rest was the previous version of the patch.


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

Is the diff view on phab broken, or am I missing something? I only see a single 
line of diff now, and don't see a way to change the diff.


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer added a comment.

I assume this is fine but I don't really understand what's going on. A test 
case would be great.


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Erik Verbruggen via Phabricator via cfe-commits]

erikjv added a comment.

@kfunk yes and yes, so @bkramer or @klimek : ping?


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Kevin Funk via Phabricator via cfe-commits]

kfunk added a comment.

@erikjv Ready for review? Does this reliably fix 
https://bugreports.qt.io/browse/QTCREATORBUG-15449?


https://reviews.llvm.org/D27810



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Erik Verbruggen via Phabricator via cfe-commits]

erikjv updated this revision to Diff 88874.
erikjv retitled this revision from "Normalize all filenames before searching 
FileManager caches" to "FileManager: mark virtual file entries as valid 
entries".
erikjv edited the summary of this revision.

https://reviews.llvm.org/D27810

Files:
  lib/Basic/FileManager.cpp


Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -386,6 +386,7 @@
   UFE->ModTime = ModificationTime;
   UFE->Dir = DirInfo;
   UFE->UID = NextFileUID++;
+  UFE->IsValid = true;
   UFE->File.reset();
   return UFE;
 }


Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -386,6 +386,7 @@
   UFE->ModTime = ModificationTime;
   UFE->Dir = DirInfo;
   UFE->UID = NextFileUID++;
+  UFE->IsValid = true;
   UFE->File.reset();
   return UFE;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits