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

Clang-Tidy: skipped warnings #2175

Closed
guwirth opened this issue Jun 2, 2021 · 9 comments · Fixed by #2177
Closed

Clang-Tidy: skipped warnings #2175

guwirth opened this issue Jun 2, 2021 · 9 comments · Fixed by #2177
Assignees
Labels
Milestone

Comments

@guwirth
Copy link
Collaborator

guwirth commented Jun 2, 2021

We have one small issue still.

/var/jenkins/0/workspace/XXX..cpp:16:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
namespace util {
^~~~~~~~~~~~~~~~
namespace util::str
/var/jenkins/0/workspace/XXX..cpp:21:29: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
  for (; (i < maxLength) && str[i]; ++i)
                            ^
/var/jenkins/0/workspace/XXX..cpp:21:29: warning: implicit conversion 'char' -> bool [readability-implicit-bool-conversion]
  for (; (i < maxLength) && str[i]; ++i)
                            ^
                            (      != 0u)
/var/jenkins/0/workspace/XXX..cpp:21:41: warning: statement should be inside braces [google-readability-braces-around-statements]
  for (; (i < maxLength) && str[i]; ++i)
                                        ^
                                         {
/var/jenkins/0/workspace/XXX..cpp:21:41: warning: statement should be inside braces [hicpp-braces-around-statements]
  for (; (i < maxLength) && str[i]; ++i)
                                        ^
                                         {
/var/jenkins/0/workspace/XXX..cpp:21:41: warning: statement should be inside braces [readability-braces-around-statements]
  for (; (i < maxLength) && str[i]; ++i)
                                        ^

                                         {

The above lines of warning is counted as 6 warnings in Jenkins warning NG plugin.

grafik

But in sonarQube its considered as only 2 warnings

grafik

PFB the skipped warnings from sonarqube debug logs:

[2021-05-31T09:07:11.601Z] 09:07:11.407 WARN: Cannot save the issue 'CxxReportIssue [ruleId=google-readability-braces-around-statements, locations=CxxReportLocation [file=/var/jenkins/0/workspace/bes-brc3-common_clang-generation/src/src/Util/String.cpp, line=21, column=41, info=statement should be inside braces], flow=]', skipping
[2021-05-31T09:07:11.601Z] 09:07:11.407 WARN: Cannot save the issue 'CxxReportIssue [ruleId=hicpp-braces-around-statements, locations=CxxReportLocation [file=/var/jenkins/0/workspace/bes-brc3-common_clang-generation/src/src/Util/String.cpp, line=21, column=41, info=statement should be inside braces], flow=]', skipping
[2021-05-31T09:07:11.601Z] 09:07:11.408 WARN: Cannot save the issue 'CxxReportIssue [ruleId=readability-braces-around-statements, locations=CxxReportLocation [file=/var/jenkins/0/workspace/bes-brc3-common_clang-generation/src/src/Util/String.cpp, line=21, column=41, info=statement should be inside braces], flow=]', skipping

Why are the warnings skipped in Sonarqube?

@guwirth guwirth added this to the 2.0.2 milestone Jun 2, 2021
@yachoor
Copy link
Contributor

yachoor commented Jun 2, 2021

Seem like they're all pointing to a column after last one - could that be the reason they cannot be added?

@yachoor
Copy link
Contributor

yachoor commented Jun 2, 2021

Other issues from clang-tidy are off by one column for me too - is one indexing from 0 and the other form 1?

@guwirth
Copy link
Collaborator Author

guwirth commented Jun 2, 2021

@yachoor thanks for the hint. That was also my first idea. Question is how Clang-Tidy is counting the columns: 0-n or 1-n?
If a sensor assigns a issue to a source file it checks if the position is available in the source, if not it ignores the issue.
Do you have a running Clang-Tidy and are able to create an issue at the first character position of a line?

@yachoor
Copy link
Contributor

yachoor commented Jun 2, 2021

It's 1-n:

/home/jchorko/test/test.cpp:9:1: warning: namespace 'test' not terminated with a closing comment [llvm-namespace-comment]
}
^

for test.cpp:

namespace test {

struct Test {
        int x;
        int y;
        int z;
}

}

@guwirth
Copy link
Collaborator Author

guwirth commented Jun 2, 2021

@yachoor thx. Problem is here:

return inputFile.newRange(line, column, line, column + 1);

1.3 version of plugin did support only lines. Because we have only one column number we have to decide if we like to mark (red underline) from position to position +1 or -1.

  • SQ counting seems to be 0 to n
  • +1 is wrong at the end of the line
  • I don't know how long the line is
  • -1 is a problem with first column but this is easy to detect

@guwirth guwirth added bug and removed question labels Jun 2, 2021
@yachoor
Copy link
Contributor

yachoor commented Jun 2, 2021

@guwirth
Also seems like an issue can be generated with column that's not in the file, but should be - from same example:

/home/jchorko/test/test.cpp:7:2: error: expected ';' after struct [clang-diagnostic-error]
}
 ^
 ;

guwirth added a commit to guwirth/sonar-cxx that referenced this issue Jun 4, 2021
- column number offset corrected: Clang-Tidy:1...n; SQ: 0...n (SonarOpenCommunity#2175)
- improved error message
- in case of invalid column number: report issue for whole line
- add test with duplicate issues
@guwirth guwirth self-assigned this Jun 4, 2021
@guwirth
Copy link
Collaborator Author

guwirth commented Jun 4, 2021

@yachoor, @dsakilesh please try with latest snapshot. Most Clang-Tidy issues should be solved now:
https://ci.appveyor.com/project/SonarOpenCommunity/sonar-cxx/branch/master/artifacts

@yachoor
Copy link
Contributor

yachoor commented Jun 5, 2021

Thank you @guwirth, works perfect - no ignored issues and indicates correct column where possible

@guwirth
Copy link
Collaborator Author

guwirth commented Jun 5, 2021

@yachoor thx for your feedback. Will wait what @dsakilesh is saying and if also positive do an official release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants