Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support gcc warnings without rule id #1708

Merged

Conversation

stalb
Copy link
Contributor

@stalb stalb commented Apr 16, 2019

close #1703

Problem:
Some gcc warnings don't have anything to match with a id rule.
In that case the default regex doesn't match and there is no id to match with a rule key

What I have done:

  • modified the regex to match gcc warnings with or without a rule is part
  • when the matched warning don't have a id part, use a default one "enabled by default".

This change is Reviewable

Copy link
Collaborator

@guwirth guwirth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stalb thanks looks good only minor issues. Please rebase and add an unit test.

public static final String DEFAULT_REGEX_DEF
= "(?<file>.*):(?<line>[0-9]+):[0-9]+:\\x20warning:\\x20(?<message>.*)\\x20\\[(?<id>.*)\\]";
= "(?<file>.*):(?<line>[0-9]+):[0-9]+:\\x20warning:\\x20(?<message>.*?)(\\x20\\[(?<id>.*)\\])?\\s*$";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all messages always in one line?

Maybe regex would be better (with id | without id)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my understanding all the warnings look either like:

  • main.c:11:6: warning: unused variable 'i' [-Wunused-variable] where the id is the corresponding switch param (-Wunused-variable) or
  • main.c:7:1: warning: no semicolon at end of struct or union where the id part is missing.

The regex match both of them.
By default warning messages are on one line, The user can force warning messages to be on several lines (if using for example -fmessage-length switch, but then current actual regex won't match either).

A named group can only appears once. So it's not possible to duplicate the beginning of the regex and if I use something like:

  • (?<file>.*):(?<line>[0-9]+):[0-9]+:\\x20warning:\\x20(?<message>.*)(\\x20\\[(?<id>.*)\\])? or
  • (?<file>.*):(?<line>[0-9]+):[0-9]+:\\x20warning:\\x20(?<message>.*)((\\x20\\[(?<id>.*)\\])|(?!(\\x20\\[.*\\]))) (matching id or not matching id)

The id is not recognized anymore.

I solved the problem using the greedy (?) and non-greedy (*?) operators and forcing to match the last part of the line.

@@ -70,6 +71,9 @@ protected String getRegex(final SensorContext context) {

@Override
protected String alignId(String id) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add short description why we need this special case.

@@ -32,8 +32,9 @@
public static final String REPORT_REGEX_DEF = "gcc.regex";
public static final String REPORT_CHARSET_DEF = "gcc.charset";
public static final String DEFAULT_CHARSET_DEF = "UTF-8";
public static final String DEFAULT_ID = "enabled by default";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normal message keys should have no blanks. Maybe we should change it in XML file? Just default.
Is Code Smell / Critical / 5min the right default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now all the gcc rules have the same type/severity/remediation effort.
I agree this should be questioned, but I suggest to address that in another PR.

At first I thought I would add a new rule, but then I found this one enabled by default, which does not correspond to any matching gcc warning id (all other id looks like -Wsomething -- i.e. the gcc activation parameter).
If you think it's better to change the id, I'm fine.

@guwirth
Copy link
Collaborator

guwirth commented Apr 25, 2019

@stalb thanks looks good except the missing unit test. Can you please add one to avoid future regressions.

@stalb stalb force-pushed the feature/gcc_warning_without_id branch from 30b4f34 to ea11d5b Compare April 25, 2019 15:16
Copy link
Collaborator

@guwirth guwirth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @stalb, thanks for providing this PR. Looks good to me. Regards

@guwirth guwirth merged commit f29a446 into SonarOpenCommunity:master Apr 25, 2019
@guwirth guwirth changed the title fix gcc warnings without rule id support gcc warnings without rule id Apr 25, 2019
@guwirth guwirth mentioned this pull request Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

not all GCC warnings are detected with the default sonar.cxx.gcc.regex
2 participants