sammccall added a comment.
This seems like the right thing in principle, but it's still pretty scary:
- this is going to break anyone who's using VFS != RealFS together with config
files, as we no longer search the old locations
- the breakage is likely to be quiet/subtle, especially if it's e.g. an obscure
flag being set in an arch-specific config file
- the fixes for "part of the toolchain is missing from VFS" tend to need code
changes in tools and tend to be hard to test
The thing the change has going for it is that I think config files and
nontrivial VFS are both pretty rarely used, so maybe there are few people using
them together.
(Google uses both, but having looked internally I don't believe we ever
actually combine them).
So I think this should land, but it needs a release note as it's a non-obvious
breaking change.
@MaskRay in case he has any thoughts about VFS use in driver in general.
================
Comment at: llvm/lib/Support/CommandLine.cpp:1268
+ if (!CurrentDir) {
+ if (auto CWD = FS.getCurrentWorkingDirectory()) {
+ CurrDir = *CWD;
----------------
the old behavior was explicitly documented ("process' cwd is used instead", not
FS's) so that documentation should be updated.
(I just talked to @kadircet about the patch that added that documentation, and
we can't think of any reason that the old behavior is actually desirable)
================
Comment at: llvm/lib/Support/CommandLine.cpp:1271
+ } else {
+ // TODO: The error should be propagated up the stack.
+ llvm::consumeError(llvm::errorCodeToError(CWD.getError()));
----------------
(This seems unidiomatic: consumeError(errorCodeToError(...)) is a no-op, we
usually spell TODO as FIXME... but it matches the surrounding code, so best
as-is I guess)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132867/new/
https://reviews.llvm.org/D132867
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
