[PATCH] D70701: Fix more VFS tests on Windows

Adrian McCarthy via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 9 16:20:42 PST 2019


amccarth marked 2 inline comments as done.
amccarth added inline comments.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431
 
-    if (IsRootEntry && !sys::path::is_absolute(Name)) {
-      assert(NameValueNode && "Name presence should be checked earlier");
----------------
rnk wrote:
> amccarth wrote:
> > rnk wrote:
> > > Is there a way to unit test this? I see some existing tests in llvm/unittests/Support/VirtualFileSystemTest.cpp.
> > > 
> > > I looked at the yaml files in the VFS tests this fixes, and I see entries like this:
> > >     { 'name': '/tests', 'type': 'directory', ... },
> > >     { 'name': '/indirect-vfs-root-files', 'type': 'directory', ... },
> > >     { 'name': 'C:/src/llvm-project/clang/test/VFS/Inputs/Broken.framework', 'type': 'directory', ... },
> > >     { 'name': 'C:/src/llvm-project/build/tools/clang/test/VFS/Output/vfsroot-with-overlay.c.tmp/cache', 'type': 'directory', ... },
> > > 
> > > So it makes sense to me that we need to handle both kinds of absolute path.
> > > Is there a way to unit test this?
> > 
> > What do you mean by "this"?  The `/tests` and `/indirect-vfs-root-files` entries in that yaml are the ones causing several tests to fail without this fix, so I take it that this is already being tested.  But perhaps you meant testing something more specific?
> > What do you mean by "this"? 
> I guess what I meant was, can you unit test the whole change in case there are behavior differences here not covered by the clang tests?
This change causes no regressions in those llvm unit tests (`llvm/unittests/Support/VirtualFileSystemTest.cpp`).  They all still pass.

But thanks for pointing those out, because my subsequent changes do seem to make a difference.


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

https://reviews.llvm.org/D70701





More information about the cfe-commits mailing list