[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:30:01 PDT 2016


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/efc75748/attachment-0001.html>


More information about the cfe-commits mailing list