haowei added inline comments.
================ Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4 + 'case-sensitive': false, + 'overlay-relative': true, + 'root-relative': 'yaml-dir', ---------------- haowei wrote: > haowei wrote: > > bnbarham wrote: > > > bnbarham wrote: > > > > I'd prefer a test without `overlay-relative` set to make it clear they > > > > don't depend on each other. > > > There's also unit tests in > > > `llvm/unittests/Support/VirtualFileSystemTest.cpp` that you could add to. > > I need some time to patch this unit test for testing the new option. > > Existing functions in this unit test file does not support defining the > > YAMLFilePath. > > > > I will update this patch when I finish. > I updated the yaml file and add unit tests with overlay-relative set to true > and false. I updated `llvm/unittests/Support/VirtualFileSystemTest.cpp` to include tests on `root-relative` option. ================ Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1903 + assert(!FullPath.empty() && "YAML file directory must exist"); + sys::fs::make_absolute(FS->getYAMLFileDir(), Name); + Name = canonicalize(Name); ---------------- bnbarham wrote: > haowei wrote: > > bnbarham wrote: > > > IMO both this and CWD should be using the base FS instead. VFS didn't > > > have a CWD previously, but now that it does it doesn't really make sense > > > to use the process wide CWD. Especially since `-working-directory` > > > doesn't change it. > > Could you clarify a bit more about the "base FS" please? I am still quite > > new to the LLVM VFS system. Which API should I use to get the appropriate > > working directory instead of the the process wide CWD? > > > > I avoided changing the default behavior (relative to the process's current > > working directory) as I am a bit concerned breaking other people's use > > cases. > > > > I looked up a bit, the value "--working-directory" will be writen into > > "VFS->setCurrentWorkingDirectory()" Do you think it is a better idea to use > > this value instead of the process current working directory? Though it > > would still be a behavior change for users rely on process current working > > directories. > > Could you clarify a bit more about the "base FS" please? > > By "base FS" I just meant the filesystem that's passed down when creating the > RedirectingFileSystem, ie. `ExternalFS` in `getVFSFromYAML`. > > > Which API should I use to get the appropriate working directory instead of > > the the process wide CWD? > > `ExternalFS->getCurrentWorkingDirectory()` and `ExternalFS->makeAbsolute` is > what I'd expect to be used here. > > > Do you think it is a better idea to use this value instead of the process > > current working directory? Though it would still be a behavior change for > > users rely on process current working directories. > > Yes, I think it's surprising that it currently *isn't* using it. It's just a > hold over from when the VFS didn't have the concept of CWD. I'd be happy with > accepting this patch as is, with a separate patch after that changes to using > the VFS CWD. > Yes, I think it's surprising that it currently *isn't* using it. It's just a > hold over from when the VFS didn't have the concept of CWD. I'd be happy with > accepting this patch as is, with a separate patch after that changes to using > the VFS CWD. I see, I can upload a follow up change after this one is landed. to use VFS CWD instead of process CWD here. It is also easier to revert if anything goes wrong. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137473/new/ https://reviews.llvm.org/D137473 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
