haowei marked 2 inline comments as done.
haowei added inline comments.
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:1010
+ std::error_code makeAbsolute(StringRef WorkingDir,
+ SmallVectorImpl<char> &Path) const;
----------------
bnbarham wrote:
> Should be private IMO
Moved it to private section and added comments
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1369
// append Path ourselves.
+ if (!sys::path::is_absolute(WorkingDir, sys::path::Style::posix) &&
+ !sys::path::is_absolute(WorkingDir,
----------------
bnbarham wrote:
> Did you find this was needed? It's already checked by the normal
> `makeAbsolute` (which checks this already) and the only other caller is when
> we're using the overlay directory path, which should always be absolute.
No, it is not needed for now. As the OverlayDir is always abs when it was set.
The normal makeAbsolute function checks "Path" param but not checking the
returned WorkDir, which is OK because the return value from
getCurrentWorkingDirectory will be abs or empty.
It is more to be future proof if someone decide to use this function for other
purpose and putting a non-abs path in WorkingDir field will causes unexpected
behaviors.
I also need to exclude empty WorkingDir though because in unit tests, the
WorkingDir is always empty.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1913
+ RedirectingFileSystem::RootRelativeKind::OverlayDir) {
+ SmallString<256> FullPath = FS->getOverlayFileDir();
+ assert(!FullPath.empty() && "Overlay file directory must exist");
----------------
bnbarham wrote:
> This is unused now. Maybe we should just merge the `else if` and `else`
> branches and just grab either `getOverlayFileDir` or
> `getCurrentWorkingDirectory` depending on `RootRelative`. They should
> otherwise be identical.
I merged these 2 else block.
I have to use sys::fs::make_absolute function for now as using FS's CWD will
break a few tests. It is better to address these test in a separate patch.
================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1875
+
+ IntrusiveRefCntPtr<vfs::FileSystem> FSOverlayRelative =
+ getFromYAMLString("{\n"
----------------
bnbarham wrote:
> Just override the previous `FS`/`S` IMO - that way we avoid the accidental
> `FS->status` a few lines down 😅
Thanks for pointing that out 😅
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
