[PATCH] D55054: [clang] Fill RealPathName for virtual files.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM and a few nits



================
Comment at: include/clang/Basic/FileManager.h:108
+
+  // Only for use in tests to see if deferred opens are happening, arther than
+  // relying on RealPathName being empty.
----------------
s/arther/rather
Let's leave out the RealPathName part, it seemed to be there for historical reasons.


================
Comment at: include/clang/Basic/FileManager.h:182
+  /// RealPathName.
+  bool fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);
+
----------------
We never use the returned value. Maybe drop it?


================
Comment at: unittests/Basic/FileManagerTest.cpp:237
   ASSERT_TRUE(file->isValid());
   // "real path name" reveals whether the file was actually opened.
   EXPECT_EQ("", file->tryGetRealPathName());
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > This test seems to rely on the old behavior. Is there a way to test the file wasn't open other than looking at RealPathName?
> There is also the vfs::file stored inside the FileEntry but unfortunately there are no getters exposing this one. But I believe this behavior is still correct for the getFile case, since `RealPathName` will only be filled when we open the file.
Using `vfs::File` LG, thanks!


================
Comment at: unittests/Basic/FileManagerTest.cpp:253
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
 }
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > We should find a way to test file was not "opened" (I think it actually does stat the file now, right?)
> AFAIK, any file accessed through getVirtualFile is immediately opened, then later on accessing a file with the same name through getFile with openfile=false won't have any affect, since this file will be alive and marked as opened in the cache.
> 
> So I believe opening and stating is dual of each other. Therefore checking for one should be equivalent to check for other.
I'm not sure about the semantics either, it seems we need to stat anyway, maybe open referes to reading the file contents in this case?
Anyhow, we're not actually changing semantics of the parts that were tested here, so there's no reason to go deeper into this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55054