[PATCH] D55054: [clang] Fill RealPathName for virtual files.
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 30 02:30:28 PST 2018
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
More information about the cfe-commits
mailing list