[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

Mon Aug 24 11:03:47 PDT 2020

hans 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.
----------------
zahen wrote:
> 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?
No, I'm not asking for it to be validated, but if e.g. execution of link.exe fails, it would be nice if the error message at least printed the path it was trying to execute so the user has a chance to see what's wrong.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85998/new/

https://reviews.llvm.org/D85998