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

Sun Aug 14 07:08:42 PDT 2016

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.

The target triple seems correct to me, but I'll see what others have to say
On Sun, Aug 14, 2016 at 1:52 AM Manuel Klimek <klimek at google.com> wrote:

>
>
> 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/667458f5/attachment.html>