Sarcasm / flycheck-irony

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

Adds error region function to pick up Irony's end column reporting #15

Closed itollefsen closed 6 years ago

itollefsen commented 8 years ago

If Irony can report the end column for a diagnostic message, then it's much simpler, and faster, to use that to calculate the error region than what Flycheck itself can do - especially if mode is `sexps'. It will also be more accurate.

This adds a custom flycheck-irony-error' struct that inherits fromflycheck-error' and adds a `column-end' member.

flycheck-irony--build-error' has been changed to build aflycheck-irony-error' instead of a flycheck-error' and setcolumn-end' based on Irony's diagnostic output.

A new error region function has been added; flycheck-irony--error-region'. That function is added as an advice around the standardflycheck-error-region-for-mode'. If the checker is "irony" and column-end' as as value,flycheck-irony--error-region' will calculate the error region, disregarding mode. If not, it will call the standard `flycheck-error-region-for-mode'.

This change depends on the changes to Sarcasm/irony-mode in pull request #312, which among other things replaces the offset in Irony's diagnostic output with the end column instead.

Sarcasm commented 8 years ago

This look like a potentially useful addition. Did you actually saw the slowness of sexps or was your change made because clang provides these regions/highlighting and you did not see a flycheck highlighting mode for the same functionality? Genuine question, just curious. I remember when I look a integrating flycheck and irony I actually wanted to have the same highlighting as clang, which in fact could be something like multi-regions + column of interest (the caret when compiling). Basically I wanted the same highlighting as here: http://clang.llvm.org/diagnostics.html (Range Highlighting for Related Text).

I'm a little bit frisky with the advising, do you think one of the following would make sense:

Do you think we could see if something can be done in flycheck upstream?

It seems like the lines highlighting mode no longer works, I only get regions now. Wouldn't it be better to override the sexp entry only?

Sarcasm commented 8 years ago

Oh, actually I found an old issue in flycheck: https://github.com/flycheck/flycheck/issues/527

Seems like a PR there would be nice, don't you think?

itollefsen commented 8 years ago

It was the slowness of sexps, not a desire for some custom highlighting.

What spurred this change was that I noticed Flycheck only marking the first character of a missing header file I had included, not the entire filename. Changing to sexps mode fixed that. However, combined with my use of the desktop package, using sexps mode when loading > ~20 files was really slow.

I have since found a decent way of turning off checking on mode-enable while loading the desktop and turning it on again when done without missing out on checking the displayed buffers and loaded ones when they're brought to the front.

I have to redo/undo my setup a bit, but I'll come back with some actual measurements.

Yes, the advice is certainly not ideal. My thinking here was that it would be a first step, a local change, that could possible be brought forward to Flycheck and dropped if Flycheck ever got a end-column marker.

I tried to read up on the link you posted, but didn't quite get all that's going on there. Certainly, if Flycheck is in the process of incorporating support for this (in one form or another), then this can be dropped. At some point, when the diagnostic output change is done, it would just be a matter of picking up the relevant parts we need for an updated version of the flycheck-error struct.

itollefsen commented 6 years ago

I've been running with this for a while, but need to update it after checking that it's still worth it. I'll close this and rather return with a updated version if need be.