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