On Sun, Aug 14, 2016, 9:52 AM Zachary Turner <ztur...@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 <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
