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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to