zahen added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:73
+ MSVCToolChain::ToolsetLayout &VSLayout) {
+ // Don't validate the input; trust the value supplied by the user.
+ // The primary motivation is to prevent unnecessary file and registry access.
----------------
hans wrote:
> zahen wrote:
> > aganea wrote:
> > > hans wrote:
> > > > What will the error look like if the user sets this but gets it wrong?
> > > > I'm sympathetic to not wanting to validate it, but if it's wrong it
> > > > would be nice if the error isn't too cryptic.
> > > @hans The flag proposed here behaves like `env VCToolsInstallDir=""
> > > clang-cl ...`, and in that case there's no validation. Do you think the
> > > path to `-vctoolsdir` should be validated and throw an error otherwise? I
> > > find it nice to completly disable the toolchain auto-detection if passing
> > > in a empty dir, without needing an additional flag, ie `clang-cl
> > > -vctoolsdir ""`.
> > ```
> > D:\>clang-cl -vctoolsdir fake main.cpp
> > clang-cl: error: unable to execute command: program not executable
> > clang-cl: error: linker command failed with exit code 1 (use -v to see
> > invocation)
> > ```
> I realize this is not because of your patch, but it would be nice if this
> error could be more clear. Ideally it would at least print the path to what's
> failing to execute. Would you be willing to add that?
It's printed when using -v as directed. I don't feel strongly against adding
the command line in all failure cases, but did want to point that out. Still
think it's worth it?
================
Comment at: clang/test/Driver/cl-options.c:686
+// vctoolsdir is handled by the driver; just check that we don't error. Pass
-c because fakedir isn't a real toolchain path
+// RUN: %clang_cl -c -vctoolsdir fakedir -- %s 2>&1
+
----------------
hans wrote:
> zahen wrote:
> > hans wrote:
> > > aganea wrote:
> > > > Check that we're not detecting a local installation, and that we
> > > > fallback to the default triple 19.11, ie.
> > > > ```
> > > > // RUN: %clang_cl ... -vctoolsdir "" -### | FileCheck %s --check-prefix
> > > > VCTOOLSDIR
> > > > // VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
> > > > ```
> > > Maybe add a comment explaining the purpose of the test.
> > >
> > > And would it be possible to have a test where -vctoolsdir is set to
> > > something, to see that it's picked up correctly?
> > What's the expectation on the test boxes? I can author such a test but it
> > would be very brittle unless we have a spec for how VS should be installed.
> I'd suggest passing -vctoolsdir "/foo" and check that some /foo dir is
> getting passed as a system include dir to cc1, and that the link.exe
> invocation is under /foo.
>
> I don't think it would be brittle.
Clever idea! I'll do it that way and key off the "ignored directory" warning
that's emitted.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85998/new/
https://reviews.llvm.org/D85998
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
