Also, compilation database support with CMake works correctly on Windows.  It generates Windows command lines which need to be tokenized using Windows command line rules (hence why this patch makes clang-tidy work on Windows)<br><div class="gmail_quote"><div dir="ltr">On Sat, Aug 13, 2016 at 10:30 PM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I'm not disagreeing that it would be nice if CMake supported this.  It's probably worth trying to do even.<br><br>But do we want to block having a working clang-tidy for clang-cl for months because of that?  Even if we can upstream the patch, how long before we up the minimum required CMake version?<br><br>The solution here does not regress behavior on any supported configuration, and adds support for a new platform.  Copying a compilation database from one machine to another seems like a hypothetical edge case.  <br><br>To support testing perhaps we can make our compilation database parser support an optional field in the json that CMake doesn't know about like PathSyntax: 'windows'.  Again, this seems like something we could do iteratively and with low priority because it's not needed in order to enable or fix any presently supported use cases.  <br><div class="gmail_quote"><div dir="ltr">On Sat, Aug 13, 2016 at 9:58 PM Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><br><div class="gmail_quote"><div dir="ltr">On Sat, Aug 13, 2016, 10:17 PM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="white-space:pre-wrap">The json is generated by CMake, so I don't thinks we can do this without patching CMake, which is a non-starter.<br></div></blockquote></div><div><br></div><div>Why? We did add compilation database support to cmake in the first place. Back then I did not support windows, btw, so I'd be surprised if that worked now without also using the arguments format that has already been added to the spec for that reason.</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="white-space:pre-wrap">I don't think this will break mingw.  Mingw is still Windows, and still uses Windows backslashes, quoting rules, and escaping rules.<br><br>You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not defined.<br><br>Reid, do you agree with this?</div></blockquote></div><div><br></div><div>I'd like to see a stronger reason why adding arguments support to cmake is not the right thing to do.</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><div class="gmail_quote"><div dir="ltr">On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">alexfh added inline comments.<br>
<br>
================<br>
Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119<br>
@@ -115,1 +118,3 @@<br>
     StringRef EscapedCommandLine) {<br>
+#if defined(LLVM_ON_WIN32)<br>
+  llvm::BumpPtrAllocator Alloc;<br>
----------------<br>
As noted on D23409, this will likely break mingw compatibility in clang on windows. We should either add a sort of auto-detection of the command line format, or extend the JSON compilation database with a way to specify the command line format used (or as Reid suggests, specify a list of arguments, but this should be done in a backward-compatible way).<br>
<br>
<br>
<a href="https://reviews.llvm.org/D23455" rel="noreferrer" target="_blank">https://reviews.llvm.org/D23455</a><br>
<br>
<br>
<br>
</blockquote></div>
</blockquote></div>
</blockquote></div></blockquote></div>