<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br></div><div><div>On Apr 26, 2013, at 11:26 , Anton Yartsev <<a href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">On 22.04.2013 23:51, Jordan Rose wrote:<br><blockquote type="cite"> Better MinGW support is certainly welcome! A few comments, though...<br><br><br>================<br>Comment at: tools/scan-build/scan-build:1563<br>@@ -1562,3 +1562,3 @@<br> $ClangCXX = $Clang;<br>-$ClangCXX =~ s/\-\d+\.\d+$//;<br>-$ClangCXX .= "++";<br>+if($^O =~/msys/) {<br>+ $ClangCXX =~ s/.exe$/++.exe/;<br>----------------<br>Not being a native Perl hacker, I found this a bit confusing. `$OSNAME` would have been a bit friendlier.<br><br>Also, this check //only// handles MinGW, correct? <a href="http://perldoc.perl.org/perlvar.html#General-Variables">http://perldoc.perl.org/perlvar.html#General-Variables</a> says that native Windows returns "MSWin32" here.<br><br>================<br>Comment at: tools/scan-build/c++-analyzer.win:3<br>@@ +2,2 @@<br>+<br>+system ("ccc-analyzer @ARGV");<br>----------------<br>The reason we have c++-analyzer at all is so that we can set run the underlying compilation with a C++ compiler. Simply forwarding c++-analyzer.win to ccc-analyzer won't actually accomplish that -- you'll have to add a flag or enviroment variable check to ccc-analyzer, which c++-analyzer.win can set before invoking ccc-analyzer.<br><br>Also, are you sure this won't destroy arguments with spaces in them? (You can test with something like `"-DSTR=macro with spaces"`.)<br><br><br><a href="http://llvm-reviews.chandlerc.com/D703">http://llvm-reviews.chandlerc.com/D703</a><br></blockquote>Thanx for the review, all of your fears were confirmed.<br>Attached is a new patch.<br><br>Temporary excluded phabricator from recipients.<br></blockquote></div><br><div>This doesn't actually fix either problem nicely. ;-) The advantage of a symlink over a wrapper script is that the indirection cost is <i>tiny...</i>at least on platforms where that trick works. That said, it does seem silly to have both a c++-analyzer symlink and a c++-analyzer.win wrapper script. How about moving the bulk of ccc-analyzer into a separate .pm file, which you can then load from ccc-analyzer and c++-analyzer? That solves the quoting problem, too.</div><div><br></div><div>(What's wrong with this quoting? Well, what if there are double quotes in the path? Better to sidestep the issue altogether.)</div><div>Jordan</div></body></html>