It occurred to me that eol detection won't work reliably. Especially for tests, someone will write a compilation database by hand and check it in. Maybe they have git set to convert newlines for them, and we don't want a test to behave differently based on your source control settings.<br><br>The target triple seems correct to me, but I'll see what others have to say<br><div class="gmail_quote"><div dir="ltr">On Sun, Aug 14, 2016 at 1:52 AM Manuel Klimek <<a href="mailto:klimek@google.com">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 Sun, Aug 14, 2016, 9:52 AM 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">I'll try and figure out who that was on Monday and loop them in. I'm not sure what problems the previous person ran into, but the test suite passes, i can run it on a large codebase, and all the results look fine and as if the tool is working. Hopefully the previous person has more insight into what I should be looking for.<br><br>It's not that it's time critical, I'm just a little surprised that it seems so important to support a use case that I'm not sure anyone has ever tried to do or would ever want to do.<br></blockquote></div><div><br></div><div>my concern is not copying files around.</div><div>My main concern is making the code more complex with a solution that only half works in the end and confusing users even more in the end vs having a solution that is well thought out and works reliably.</div><div>If we're sure enough eol detection will work reliably, I'm fine. So far all attempts I've seen had some problems.</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>The idea of copying a compilation database around across machines, is this really something we need to go out of our way to support?<br><div class="gmail_quote"><div dir="ltr">On Sat, Aug 13, 2016 at 11:34 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">For years nobody worked on Windows support, so I'm somewhat surprised this is suddenly time critical.<div>Usually the way this works is that we add the feature to upstream cmake, and then make a recent enough cmake a requirement for tooling. There's no need to make it a requirement for anything else. That has worked well for the initial version, too.</div><div><br></div><div>My main problem is that I'm surprised you say you got a working version at all, given all the differences. I'm on vacation, but afterwards I'm happy to look up who previously worked on this and loop them into this thread. Or you can figure out who that was (they added the arguments support iirc) and make sure they're signing of on this.</div><div><br><div dir="auto"><br><br><div class="gmail_quote"><div dir="ltr">On Sun, Aug 14, 2016, 8:52 AM 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">According to the existing spec [<a href="http://clang.llvm.org/docs/JSONCompilationDatabase.html" target="_blank">http://clang.llvm.org/docs/JSONCompilationDatabase.html</a>], the command field "must be a valid command to rerun the exact compilation step for the translation unit in the environment the build system uses.".<br><br>So copying compilation databases across environments with incompatible command line syntaxes is already against spec.<br><br>That said, this does make me think that perhaps we should be checking the target triple instead of the host compilation environment, because if you were to cross compile clang-tidy for Windows from linux then try to use that clang-tidy on Windows, it would expect unix paths.<br><br>How does that sound?<br><div class="gmail_quote"><div dir="ltr">On Sat, Aug 13, 2016 at 10:31 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">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" 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">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></blockquote></div>
</blockquote></div></div></div></blockquote></div>
</blockquote></div>
</blockquote></div>