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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: include/clang/Basic/FileManager.h:177
+  /// Fills the RealPathName entry in UFE by converting the given filename to an
+  /// absolute path, returns true if FileName was modified.
+  bool fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);
----------------
The FIXME inside the function suggests we might change the name or semantics of RealPathName, so maybe simply refer to the field by name and avoid mentioning the semantics of the function.


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


================
Comment at: unittests/Basic/FileManagerTest.cpp:238
   // "real path name" reveals whether the file was actually opened.
   EXPECT_EQ("", file->tryGetRealPathName());
 
----------------
Should we fill-in the RealPathName in that case as well?
I'm not sure what the semantics should be. WDYT?


================
Comment at: unittests/Basic/FileManagerTest.cpp:253
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
 }
----------------
We should find a way to test file was not "opened" (I think it actually does stat the file now, right?)


================
Comment at: unittests/Basic/FileManagerTest.cpp:327
+
+  EXPECT_EQ(file1->tryGetRealPathName(), file2->tryGetRealPathName());
 }
----------------
After a check `EXPECT_EQ(file1, file2)`, this is clearly true.
Maybe add a smaller test with a single call to `getVirtualFile()` and an assertion that the real path is set?
Alternatively, add an assertion to this test right after the first call to `getVirtualFile()`.



Repository:
  rC Clang

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

https://reviews.llvm.org/D55054