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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
