[PATCH] D23455: [Tooling] Parse compilation database command lines properly on Windows

Zachary Turner via cfe-commits cfe-commits at lists.llvm.org
Sat Aug 13 22:52:39 PDT 2016

According to the existing spec [
http://clang.llvm.org/docs/JSONCompilationDatabase.html], 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.".

So copying compilation databases across environments with incompatible
command line syntaxes is already against spec.

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.

How does that sound?
On Sat, Aug 13, 2016 at 10:31 PM Zachary Turner <zturner at google.com> wrote:

> 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)
> On Sat, Aug 13, 2016 at 10:30 PM Zachary Turner <zturner at google.com>
> wrote:
>> I'm not disagreeing that it would be nice if CMake supported this. It's
>> probably worth trying to do even.
>> 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?
>> 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.
>> 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.
>> On Sat, Aug 13, 2016 at 9:58 PM Manuel Klimek <klimek at google.com> wrote:
>>> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner <zturner at google.com>
>>> wrote:
>>>> The json is generated by CMake, so I don't thinks we can do this
>>>> without patching CMake, which is a non-starter.
>>> 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.
>>> I don't think this will break mingw. Mingw is still Windows, and still
>>>> uses Windows backslashes, quoting rules, and escaping rules.
>>>> You might be thinking of cygwin, but in the case LLVM_ON_WIN32is not
>>>> defined.
>>>> Reid, do you agree with this?
>>> I'd like to see a stronger reason why adding arguments support to cmake
>>> is not the right thing to do.
>>>> On Sat, Aug 13, 2016 at 10:35 AM Alexander Kornienko <alexfh at google.com>
>>>> wrote:
>>>>> alexfh added inline comments.
>>>>> ================
>>>>> Comment at: lib/Tooling/JSONCompilationDatabase.cpp:119
>>>>> @@ -115,1 +118,3 @@
>>>>>      StringRef EscapedCommandLine) {
>>>>> +#if defined(LLVM_ON_WIN32)
>>>>> +  llvm::BumpPtrAllocator Alloc;
>>>>> ----------------
>>>>> 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).
>>>>> https://reviews.llvm.org/D23455
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160814/df658c8b/attachment.html>

More information about the cfe-commits mailing list