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

[Manuel Klimek via cfe-commits]

On Sun, Aug 14, 2016, 9:52 AM Zachary Turner <zturner at google.com> wrote:

> 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.
>
> 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.
>

my concern is not copying files around.
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.
If we're sure enough eol detection will work reliably, I'm fine. So far all
attempts I've seen had some problems.


> The idea of copying a compilation database around across machines, is this
> really something we need to go out of our way to support?
> On Sat, Aug 13, 2016 at 11:34 PM Manuel Klimek <klimek at google.com> wrote:
>
>> For years nobody worked on Windows support, so I'm somewhat surprised
>> this is suddenly time critical.
>> 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.
>>
>> 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.
>>
>>
>>
>> On Sun, Aug 14, 2016, 8:52 AM Zachary Turner <zturner at google.com> wrote:
>>
>>> 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/73b9a7eb/attachment.html>