dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land.
In D109128#3097060 <https://reviews.llvm.org/D109128#3097060>, @keith wrote: > @dexonsmith turns out the test I was adding is broken on main today too. If > it's alright with you I will punt that to another diff to not increase the > scope of this one. Yes, SGTM. In D109128#3113058 <https://reviews.llvm.org/D109128#3113058>, @keith wrote: > @dexonsmith can you take another look? LGTM! Thanks for seeing this through. (And thanks for your patience; this (and the pings) just got buried somehow in my inbox.) ================ Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1738 + +TEST_F(VFSFromYAMLTest, RelativePathHitWithoutCWD) { + IntrusiveRefCntPtr<vfs::InMemoryFileSystem> BaseFS( ---------------- keith wrote: > So this test case is actually failing. The difference between it and the > others is I don't call `FS->setCurrentWorkingDirectory("//root/foo")`. This > results in us (with my most recent change here) performing this logic: > > 1. Fetch the absolute //root/foo/vfsname > 2. This results in `realname` being returned > 3. We attempt to canonicalize `realname`, but we have no `pwd`, so this > doesn't result in a valid path > 4. everything fails past this > > It seems to me, without having a ton of context here, that the value returned > from the VFS lookup should actually be `//root/foo/realname`, since otherwise > we could likely hit one of the same issues as those discussed above where if > you actually had this situation: > > - `mkdir /root/foo /root/bar` > - `touch /root/foo/realname /root/bar/realname` > - `cd /root/bar` > - lookup `/root/foo/vfsname`, get back `realname` > - canonicalize `realname` to the wrong path `/root/bar/realname` > > You'd end up with the wrong file, but I don't think this is actually new > behavior with my change? > > @dexonsmith wdyt? I agree with your analysis of the correct behaviour. Until the first call to `setCurrentWorkingDirectory()`, it seems like the working directory should implicitly be the one in the underlying FS (not sure whether it should be captured on construction, or just used going forward). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
