Sarcasm / flycheck-irony

C, C++ and Objective-C support for Flycheck, using Irony Mode
56 stars 10 forks source link

Integrate flycheck-clang-analyzer #18

Closed alexmurray closed 7 years ago

alexmurray commented 7 years ago

I recently created a flycheck checker, flycheck-clang-analyzer which leverages the existing irony-db for a project to then invoke clang --analyze to get per-file, quick static analysis (compared to doing more offline full scan-build analysis of an entire project with only html output etc).

However, the more I think about it, I wonder should / could this be integrated directly into irony, or flycheck-irony? If nothing else, would be interested to get your thoughts on it, and whether if nothing else you could mention it with the irony project in general? I think it provides a very useful and compelling enhancement.

Sarcasm commented 7 years ago

Wow, that's great, this is something I wanted to try, I was aware of flycheck-clangcheck which is able to analyze.

To answer your question, I initially wanted "irony" to integrate many tools, mostly clang-based, but it could be something else, for example calling the compiler to disassemble, invoking cppcheck, ... So this is a welcome addition. I think you took the right approach by making an independent package, as there is no need for tight coupling with irony mode, what is important is the access to compile options (irony-cdb).

I think the package is fine on its own, but I also think merging it in flycheck-irony repo is ok. Once you make a decision about what you prefer, and we know the URL to reference, I will be happy to mention it on irony-mode README's.

Honestly, I think it's best if this does not rely too much on irony-mode, and that irony-mode is just a provider of compile options. Users could prefer to use rtags as a provider, or maybe some emacs-compdb package that I'm thinking about publishing.

alexmurray commented 7 years ago

Thanks for the kind words - I'm happy to keep it separate - I just want to try and get as much exposure for it as possible, since I have a strong opinion on the usefulness of static analysis for c/c++ and especially for trying to make it as easy as possible to provide results to developers as they code, I think this is quite a game changer - I know it has been for me :)

So if you are happy to maintain it then for sure, please merge / integrate as you feel appropriate to flycheck-irony - otherwise I am happy to keep it separate.

Let me know your thoughts - and if you have tried it, would be great to know if it works for you and if you find it useful.

Sarcasm commented 7 years ago

Here are some feedback of my first try at the package:

alexmurray commented 7 years ago

Thanks for the feedback. For the first issue, we can either change the checker to only run on a saved buffer, which might make sense given it takes in the order of seconds to run for me at least anyway, and this should hopefully ensure the buffer is more sane.

I already automatically configure the checker to be chained after flycheck-irony, and that flycheck-irony must have no errors (warnings are okay): https://github.com/alexmurray/flycheck-clang-analyzer/blob/master/flycheck-clang-analyzer.el#L88

I have not used clang-tidy (I am just using clang-3.8 as shipped in Ubuntu 16.04) so haven't looked at that.

A few questions:

Would it be worth moving this to a new issue at https://github.com/alexmurray/flycheck-clang-analyzer rather than here for flycheck-irony? I realise now there probably isn't much point continuing the discussion of whether to integrate this in flycheck-irony or not until we resolve these issues.

alexmurray commented 7 years ago

Also - to answer your question about flycheck-clangcheck - clang-check expects a compilation database so can't be used easily when using the .clang_complete approach - whereas clang --analyze allows to specify compilation arguments on the command-line so can be used for all backends.

Sarcasm commented 7 years ago

Ok, great for the chaining, I can confirm it works.

Regarding your questions, and my initial issue, it was due to the diagnostic colors (as you guessed I think), I opened a bug https://github.com/alexmurray/flycheck-clang-analyzer/issues/1

What irony backend are you using? I generally only use the .clang_complete one so perhaps there is some difference when using the json backed or other?

I used a JSON compilation database, and was testing on irony-server (adding intentional errors).

Would it be worth moving this to a new issue at https://github.com/alexmurray/flycheck-clang-analyzer rather than here for flycheck-irony?

Agreed.

alexmurray commented 7 years ago

Ok I think I have reached a conclusion on whether to integrate it or not - I have decided to try and support both irony and rtags as backends so then it doesn't make as much sense to integrate with flycheck-irony, so will keep it a separate package.

Thanks for your help though - also thought I should say, I am a long-time user of irony, and love the almost zero setup it brings. It really makes C/C++ dev with Emacs a pleasure - so thanks for all your hard work on it.

Sarcasm commented 7 years ago

Great, you made the right decision IMHO, I have enabled the checker in my config.