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. 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 <kli...@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 <ztur...@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 <ztur...@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 <ztur...@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 <kli...@google.com> >>>> wrote: >>>> >>>>> >>>>> >>>>> On Sat, Aug 13, 2016, 10:17 PM Zachary Turner <ztur...@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 < >>>>>> ale...@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 >>>>>>> >>>>>>> >>>>>>> >>>>>>>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
