bnbarham added a subscriber: dexonsmith.
bnbarham added a comment.
This seems reasonable to me in general. @dexonsmith in case you have any
thoughts.
================
Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4
+ 'case-sensitive': false,
+ 'overlay-relative': true,
+ 'root-relative': 'yaml-dir',
----------------
I'd prefer a test without `overlay-relative` set to make it clear they don't
depend on each other.
================
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>
----------------
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 :)
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:752
+ enum class RootRelativeKind {
+ /// The roots are relative to the current working directory.
+ CWD,
----------------
`to the current working directory when the overlay is created.` is maybe a
little clearer to me
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:755
+ /// The roots are relative to the directory where the YAML file locates.
+ YAMLDir
+ };
----------------
Any thoughts on something like `OverlayDir` instead?
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:926
+ /// is set. This will also be prefixed to each 'roots->name' if RootRelative
+ /// is set to RootRelativeKind::YAMLDir.
+ std::string YAMLFileDir;
----------------
*and the path is relative*
================
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);
----------------
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.
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
