bnbarham added inline comments.
================ Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:660 /// 'use-external-names': <boolean, default=true> +/// 'root-relative': <string, one of 'cwd' or 'yaml-dir', default='cwd'> /// 'overlay-relative': <boolean, default=false> ---------------- haowei wrote: > bnbarham wrote: > > phosek wrote: > > > Could we make this just a boolean akin to `overlay-relative` since there > > > are only two options (default to `false`)? > > I personally prefer being explicit here, `overlay-relative` is fairly > > confusing as it is. > > > > `overlay-relative` isn't about allowing relative paths, but instead means > > that *all* external paths should be prefixed with the directory of the > > overlay. To put another way, external paths can be relative whether this is > > true/false, `overlay-relative` just *always* prepends the overlay path. > > > > Could you add a comment to make it clear that this has no interaction with > > `overlay-relative`? If you want to add a comment to `overlay-relative` with > > something like the above that would also be appreciated :) > I also think `overlay-relative` is a bit misleading and it should be an enum > instead of a boolean option. And it should only work if the external paths > are relative instead of blindly prepend the overlay dir to every external > paths. But changing this will be a breaking change so I prefer to avoid it in > this patch. > > I updated the descriptions. FWIW it was added explicitly to always prepend, it's used for reproducers where the overlay may already have absolute paths. I was just trying to explain what `overlay-relative` does. ================ 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); ---------------- 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. 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
