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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
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());
----------------
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.


================
Comment at: unittests/Basic/FileManagerTest.cpp:238
   // "real path name" reveals whether the file was actually opened.
   EXPECT_EQ("", file->tryGetRealPathName());
 
----------------
ilya-biryukov wrote:
> Should we fill-in the RealPathName in that case as well?
> I'm not sure what the semantics should be. WDYT?
I don't think so, since we need the vfs::file to query the real path, I believe it is still ok to leave them empty for real files that hasn't been opened yet.

IIUC, stating and opening are very similar in nature(from the perspective of FileManager), the only difference is there is no deferred stating for virtual files therefore it is like a normal file that will always be opened.


================
Comment at: unittests/Basic/FileManagerTest.cpp:253
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
 }
----------------
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55054