Compiler error regular expression does match user defined expression and default

[torsten-rupp]

If in the build settings a compiler error regular expression is defined, both that expression as well as the default behaviour is used to identify error file name and line number in compiler output lines. This lead for e. g. the following output from a gcc

In file included from /home/torsten/projects/bar/bar/common/configvalues.c:33:0:
/home/torsten/projects/bar/bar/common/configvalues.h:676:2: warning: #warning obsolete [-Wcpp]

to 2 errors resp. warnings in the message window, but only the second line is a warning.

I identified the problem is probably in the function

msgwindow.c: msgwin_parse_compiler_error_line()

where filetypes_parse_error_message() is called. If it returns FALSE the default way to identifier compiler error/warning messages without the regular expression is called. This is even the case when the user regular expression is valid, but does not match to a particular line.

Short: the function

filetypes_parse_error_message()

should return TRUE, too if the regular expression does not match resp. the called function g_regex_match() return FALSE.

[torsten-rupp]

A simple patch for 1.37.1 would be:

diff --git a/src/filetypes.c b/src/filetypes.c
index 3d2a1e3a..2b17fa66 100644
--- a/src/filetypes.c
+++ b/src/filetypes.c
@@ -1306,7 +1306,7 @@ gboolean filetypes_parse_error_message(GeanyFiletype *ft, const gchar *message,
        if (!g_regex_match(ft->priv->error_regex, message, 0, &minfo))
        {
                g_match_info_free(minfo);
-               return FALSE;
+               return TRUE;
        }
 
        n_match_groups = g_match_info_get_match_count(minfo);

[elextr]

For C++ gcc (and clang) all the places in the "error traceback" (as I call it) need to be highlighted as clickable since its most likely that the error is in the user code, not in the nested depths of the standard library template where the error occurred during expansion.

[torsten-rupp]

I think if a user regular expression is given only lines matching this expression should be highlighted and clickable. With the current implementation the user regular expression just "extend" the matched strings. It does not replace it. E. g. it is not possible to declare exactly only to detect those lines which match to the expression. This lead to false detected error messages and e. g. also report warnings as errors (a second regular expression for warnings would be nice, too).

[elextr]

Yes, the regular expression just expands the scope of detection since, as I said above, its better to have more lines clickable than less.

#1649 is a bitrotted PR to distinguish warnings that nobody has reviewed and tested, so I guess its not that important to anybody.

[torsten-rupp]

The problem is with "more lines" is that the menu function "Next error" does not work as expected. It does not show the next error, but just the next warning or some other line which look like <filename>:<number>. How about to be able to click any line and just handling lines matching the expression as lines which can be found by "Next error"?

In the documentation it is written:

error_regex

    This is a Perl-compatible regular expression (PCRE) to parse a filename (absolute or relative) and line number from the build output. If undefined, Geany will fall back to its default error message parsing.

This is at least missleading. Geany does not fall back, it always use the default error message parsing if the user defined regular expression does not match.

[torsten-rupp]

BTW: it would be easy to get as much lines as needed with an apropiated user regular expression.

[elextr]

Actually the code does exactly what it says in the manual, see

geany/src/msgwindow.c

Line 1114 in b524a58

if (!filetypes_parse_error_message(ft, trimmed_string, filename, line))

so I suspect your regex isn't matching properly.

[torsten-rupp]

The regular expression is OK. The problem I have is that I get more red lines than the expression is matching. The problem is that filetypes_parse_error_message() also return FALSE if a line do NOT match to the expression. E. g. if the expression is something like

(foo):(123)

then the compiler output lines

foo:123
/abc:456

will match. The line "foo:123" is matched with the regular expression, the line "/abc:456" is matched by the default parsing implementation. The manual says the default parsing is used if the regular expression is undefined. But the default parsing is used also if the regular expression is defined and valid, but just do not match.

I tried this expression:

^([^:]+):([0-9]+):[0-9]+:\serror:.*

This matches lines like <filename>:<number>:<number>: error. I could verify this when I commented out the default parsing in line 1117 (the call to parse_compiler_error_line). Then only these lines become red and clickable in the compiler tab.

[elextr]

Ahh, you are right, I thought it was additive, but then misinterpreted the code.

Ok the manual needs correcting then, care to make a PR?

[torsten-rupp]

Thank you.

I would prefer not to correct the manual, but to change line 1114 resp. the called match function filetypes_parse_error_message to give the regular expression the only priority - if it is defined and valid. Then both use cases are covered: a) use the default implementation to match as much lines as possible and b) use the user defined regular expression to match only those lines which match the expression. This would make it IMHO more clear to the user in the build dialog settings,too, that the regular expression is responsible for finding compiler error messages or warnings when someone define one.

[elextr]

I dunno, its been like that since before my time (eg that line in 0.18 is the same).

The purpose of the parsing is to identify lines in the compiler output containing filename and line number that the user might want to go to so they can investigate and fix the error. That is used to make the message line clickable to go to that code line. Its a user convenience.

Like I said above I believe the idea is to make as many lines as sensibly possible be clickable (ie file & line are detected no matter by which method) to allow the user to go to the any of those lines. Since many compilers spit out lots of lines for each error and the fix may be on any one of those lines. Lines that don't have file and line detected cannot be clickable because there is no indication of where to go, but all lines that have an identifiable filename and line number should be clickable even if they pertain to the same error. And as the extra lines often have subtly varying formats the ability to simply add detection for those on top of the standard detection is useful.

I wouldn't like to see any lines that are currently clickable be made un-clickable, that would be a significant reduction in functionality, and all the other IDEs that I have recently used allow any line that has a recognised file and line in it to be clicked to go to that line, no matter what the message was.

Agreed in the current circumstances next/previous error action actually goes to next/previous line that is clickable, since in compilers for simple languages or older compilers thats usually the next error, but for the obnoxiously verbose modern C/C++ compilers its not the case. But that would be harder to fix.

The only case I can see to not fallback if a regex fails is if the fallback is producing a lot of cases where it detects a filename and line number when its not one. That would be better disabled with a checkbox per filetype (even better per regex) rather than blowing away the functionality for all filetypes hard coded. Just because you don't want it shouldn't mean everyone loses it.

Or the other option is to totally remove the fallback code and provide an equivalent default regex in every filetype, PRs are welcome.

[torsten-rupp]

Maybe we could add a checkbox besides the error regular expression input field in the build settings dialog (which is by default "enabled") indicating that the hard coded detection should be used, too? Then those users would want to work like they do it up to now can just continue like this. Users who want to control the detected lines by the regular expression could enter the regular expression they want and optionally uncheck the checkbox to disable the hard coded detection.

Adding a checkbox and storing that value into the settings will require more changes. If such a change would be acceptable by you I can try if I'm able to create a patch with that (of a push request/branch resp. a pull request when I will know how to do that).

[torsten-rupp]

Attached is a view of the build settings dialog with the new default checkboxes. Would this be acceptable?

[torsten-rupp]

Will not implement this anymore. Will write my own plugin.

[elextr]

Why did you not make a pull request with your suggested changes?

But certainly it does not appear to have much support, although its often the case that people need to see touch and smell the change before they can consider it.

But of course its always your choice.

[torsten-rupp]

Dear elextr,

Why did you not make a pull request with your suggested changes?

Because I did not get any answer if the suggestion I made is OK. I do
not like to spent time and implement something if there is no commitment
that there is a real chance that it may be accepted.

But certainly it does not appear to have much support, although its
often the case that people need to see touch and smell the change before
they can consider it.

I think I made a proper suggestion including a screenshot from a
click-dummy to show how it looks like and how it will work. If you still
think it is a good idea, I can continue with that. But currently I spend
my time in development of a plugin.

[elextr]

Because I did not get any answer if the suggestion I made is OK.

Well, I had initially proposed the settings, and you added a GUI suggestion, so you could take it that I was not opposed to it.

But having said lots I left it it to see what others thought, there are other contributors to Geany.

But, in the current world situation its not known how these various contributors to Geany are going, many have not been heard from for some time, and some countries are doing it tough, so I would expect that volunteer contributions to free software projects are not the top of their priority list. Maybe a picture and some words are possibly not motivating enough to comment if its never been a problem for them.

That doesn't help you much I agree, but its the state of the world.

But even in a perfect world you would never get a guarantee of acceptance before the PR, at most a "might" be accepted.

Still, having an actual pull request may be more enticing to others to comment, and would provide more information on how it would work.

Re-implementing the compile/build system in a plugin is a fair amount of work, just to get next/previous error to skip some lines. But as I said, its your choice whats most important to you.

[torsten-rupp]

Thank you for your reply.

It is only not a waste of time for me if we agree that the proposed function may be accepted. As I have written. Exactly because my offered contribution is not the top in my priority list, it is important to avoid wasting time whenever possible. Of course I know there is no guaranty for an acceptance, but if there is not an agreement at the start it is a waste of time to create a pull request and then just to get the answer "no it is not what i meant". The proposed modification is not a big thing and I agree to you that the existing functionality should not be effected. Therefore the extended proposal with the checkboxes: they do exactly that. If they are set (which will be the default) the behaviour is exactly like it is now. My original idea was not to get too many found "errors" because the pattern matching for the compilers output is implemented additive and not only what is specified in the option settings for the regular expressions. This behaviour could be controlled by the check boxes.

And BTW: our positions for this small modfication are not as far away from each other than it may look like.

The replacement plugin is something more than the proposed extension here: it shows errors and warnings separated in a tree structure where the sometimes long error output of the compiler can be folded, the entries can be clicked to jump to the error/warning location (same like the integrated built function), line indicators as now mark the exact line in the window (also the same like now). This is already working in the current state. What is left to implement: all global and project specific settings and maybe some menu.

[torsten-rupp]

I will wait for comments from other contributors if this extension is useful.

[torsten-rupp]

I created a pull request for this:

#2858