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

CppCheck cleanup #975

Merged
merged 1 commit into from
Nov 7, 2017
Merged

CppCheck cleanup #975

merged 1 commit into from
Nov 7, 2017

Conversation

fallberg
Copy link
Contributor

@fallberg fallberg commented Nov 5, 2017

  • Add support for inline cppcheck suppressions
  • Clean out all known cppcheck issues
  • Terminate toll-gate on found cppcheck issues

@fallberg fallberg force-pushed the cppcheck-fixes branch 2 times, most recently from fbec7ad to 5830001 Compare November 5, 2017 20:29
ConfigurationNotChecked
unmatchedSuppression
unreadVariable:*/MyHwNRF5.cpp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this because the error is in a macro so it throws off the in-line suppression logic of cppcheck

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explaining why. Should the explanation be inline with the code instead of being in a comment in a review? Or would having inline comments clutter the code too much? We prefer having (doxygen)documentation inline, so maybe we should have cppcheck exceptions inline as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if the cfg file supports comments. I have not been able to verify that.

Copy link
Member

@mfalkvidd mfalkvidd Nov 6, 2017

Choose a reason for hiding this comment

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

Seems like it does. From the cppcheck manual:

Listing suppressions in a file

You can create a suppressions file. Example:

// suppress memleak and exceptNew errors in the file src/file1.cpp
memleak:src/file1.cpp
exceptNew:src/file1.cpp
// suppress all uninitvar errors in all files
uninitvar

Note that you may add empty lines and comments in the suppressions file.

@@ -49,7 +49,9 @@ MyMessage _ethernetMsg;
#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))

typedef struct {
// cppcheck-suppress unusedStructMember
Copy link
Member

Choose a reason for hiding this comment

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

Should we (manually, during code review I guess) encourage that the annotations are accompanied by a comment explaining why the suppression was added?

To take this code line as example, why do we want to suppress the warning?

If we decide to encourage commenting, should the comment be before or after the annotation? On the same line or above/below? /* style comment or // ? Consistency would make it easier for code contributors and reviewers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add further comments on the inline suppressions. Most of these are due to them being initialized in a init function, and to avoid claiming unnecessary memory because the class is instantiated but not used.

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 this case, I think it is a matter of the inputBuffer is used in a rats-nest of #ifdef:s which falls between the masks of the cppcheck grid (it will not evaluate ALL possible combinations of preprocessor flags).

@fallberg fallberg force-pushed the cppcheck-fixes branch 3 times, most recently from 1f56ef9 to 9fe597c Compare November 6, 2017 20:48
@tekka007 tekka007 added this to the 2.2.0 milestone Nov 6, 2017
* Add support for inline cppcheck suppressions
* Clean out all known cppcheck issues
* Terminate toll-gate on found cppcheck issues
@tbowmo tbowmo merged commit 2d5404d into mysensors:development Nov 7, 2017
@fallberg fallberg deleted the cppcheck-fixes branch November 7, 2017 19:44
pgollor pushed a commit to pgollor/MySensors that referenced this pull request Dec 3, 2017
* Add support for inline cppcheck suppressions
* Clean out all known cppcheck issues
* Terminate toll-gate on found cppcheck issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants