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',
----------------
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.
================
Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4
+ 'case-sensitive': false,
+ 'overlay-relative': true,
+ 'root-relative': 'yaml-dir',
----------------
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.
================
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>
----------------
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.
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:752
+ enum class RootRelativeKind {
+ /// The roots are relative to the current working directory.
+ CWD,
----------------
bnbarham wrote:
> `to the current working directory when the overlay is created.` is maybe a
> little clearer to me
Added into the comments.
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:755
+ /// The roots are relative to the directory where the YAML file locates.
+ YAMLDir
+ };
----------------
bnbarham wrote:
> Any thoughts on something like `OverlayDir` instead?
I think OverlayDir is better.
================
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:
> 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.
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
