Code quality issues identified by SonarQube #29
Conversation
…'d within a block
… be removed cpp:S1854 MITRE CWE-563 CERT MSC13-CPP Dead stores should be removed c:S3626 Jump statements should not be redundant cpp:SingleDeclarationPerStatement MISRA C++:2008 8-0-1 CERT, DCL04-CPP Init-declarator-lists and member-declarator-lists should consist of single init-declarators and member-declarators respectively
|
I am wondering why "Preprocessor directives should not be indented", if it eases the readability of code? |
|
Also, why "Init-declarator-lists and member-declarator-lists should consist of single init-declarators and member-declarators respectively"? |
| @@ -1090,47 +1090,6 @@ namespace Dashel | |||
| }; | |||
|
|
|||
|
|
|||
| /* | |||
There was a problem hiding this comment.
I think this block of code should be moved into an example.
|
Also I would use const reference for exceptions. |
|
Beside these, ok to merge. |
|
We can add exceptions to the rules, especially if there is already a different community standard in the code. That's why I removed the rule that namespaces should start with lower case [a-z], for example. Here is the documentation for the two rules you mentioned: |
|
For the variable declarations separated by a comma, I understand the argument that it might be confusing for some developers who are not used to this feature of C++, so I'm fine with enforcing this rule. For Preprocessor directives should not be indented, I agree for the change to put the #ifdef without indentation, the rationale makes sense. However, I like the indented includes within the #ifdef, to separate them from the #ifdef itself. What is your personal opinion on that? Also, I'm more than happy to merge the changes we agree on. |
|
I loathe the preprocessor and I like the rule to not indent Preprocessor directives with conditional logic make me nervous because I think that most of the time the configuration system is better informed. For example, L77, I should have left the decision to CMake, whether TARGET_OS_IPHONE depended on MACOSX or not.
If I understand your question, you are referring to the conditional includes L71, L94, etc. In those cases the scoping is the preprocessor itself, and indenting doesn't give a false impression. So if it really improves readability we could manually allow exceptions to the rule in those places. |
|
Rereading my answer, I didn't mean to be passive-aggressive. My opinion is that if you already have a convention then there is no compelling reason to change it. |
|
f30e48c catches exceptions with const references |
Code quality
I have started running SonarQube on the Aseba and Enki code bases, and here is a PR to discuss the improvements to code quality that SonarQube suggests. It includes #28. Each commit in this PR addresses a separate rule; for example, 82303c7 addresses rule cpp:S3230. It should be possible to cherry-pick individual commits. This PR resolves roughly half of the issues identified in the Dashel code.
Remaining issues
The SonarQube report at https://sonarcloud.io/dashboard?id=org.thymio%3Adashel is up to date with this PR, and shows the issues that remain after this PR is applied. The following is a list of the most frequent rules that are still triggered. (Note that each rule page ends with a link to specific issues)
This rule is triggered by the complex class hierarchy in Dashel and the need for
protectedaccess to attributes. It might be appropriate to ignore this rule.This rule can probably be ignored, since the Aseba convention is to specify access separately for attributes and methods:
Finally, two functions are identified as being too complex. At a minimum unit tests (#27) need to be defined before addressing this problem.