Skip to content

cppcheck: Provide consistent reason formatting for all cppcheck-suppresses#6024

Merged
x3ro merged 1 commit intoRIOT-OS:masterfrom
x3ro:cppcheck-suppress-explained
Oct 10, 2017
Merged

cppcheck: Provide consistent reason formatting for all cppcheck-suppresses#6024
x3ro merged 1 commit intoRIOT-OS:masterfrom
x3ro:cppcheck-suppress-explained

Conversation

@x3ro
Copy link
Copy Markdown
Contributor

@x3ro x3ro commented Oct 30, 2016 •
edited
Loading

I tried to tackle #1895 by adding a rationale to all suppressions that were not explained before, and also re-formatted all existing explanations to match the format:

/* cppcheck-suppress <suppression>
 * (reason: <reason>) */

I would propose this format to be our "standard format", since previously everyone kind of did their own thing which sometimes made it hard to see where the reasoning was and which suppression it belonged to. I also removed suppressions which my current cppcheck (1.76.1) did not complain about. Let's see what Murdock says.

There are also two suppressions marked with TODO, I'd appreciate help trying to explain them properly :)

Fixes #1895

PS: I've added a guideline regarding the format to the coding conventions

@x3ro x3ro added Area: doc Area: Documentation Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 30, 2016
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 30, 2016

Does this even work consistently? To my experience cppcheck (or at least an earlier version) is complaining if the suppression is not directly in the line above.

miri64
miri64 reviewed Oct 30, 2016
View reviewed changes
cpu/x86/x86_pci.c Outdated
bus, dev, fun, vendor_name, device_name, baseclass_name, subclass_name, class.revision_id);

/* cppcheck-suppress memleakOnRealloc */
/* c1ppcheck-suppress memleakOnRealloc TODO */
Copy link
Copy Markdown
Member

@miri64 miri64 Oct 30, 2016

Choose a reason for hiding this comment

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

typo?

Copy link
Copy Markdown
Contributor Author

@x3ro x3ro Oct 30, 2016

Choose a reason for hiding this comment

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

I used that for disabling the suppressions and checking whether cppcheck actually complained and forgot to take it out. But since its one of the TODOs it doesn't matter that much :) Will fix once I have a good reason to add!

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 30, 2016

Please properly prefix both PR and commit message ;-)

@miri64 miri64 self-assigned this Oct 30, 2016
@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Oct 30, 2016

Would this be prefixed doc?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 30, 2016

Would this be prefixed doc?

how about cppcheck or lint?

@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Oct 30, 2016

Regarding whether or not it works consistently: seems to work with the version I had and with what Murdock uses. I think the "directly below" thing is correct, but it must be directly below the comment? That's my best guess :D

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 30, 2016 •
edited
Loading

Regarding whether or not it works consistently: seems to work with the version I had and with what Murdock uses. I think the "directly below" thing is correct, but it must be directly below the comment? That's my best guess :D

I don't know why, but for some reason has cppcheck set the error code for modified files to 0, so there are actually errors:

Running './dist/tools/cppcheck/check.sh master --diff-filter=MR --error-exitcode=0' ✓
Command output:

    cpu/x86/x86_pci.c:222: error (memleakOnRealloc): Common realloc mistake: 'known_pci_devices' nulled but not freed upon failure sys/include/timex.h:82: performance (passedByValue): Function parameter 'a' should be passed by reference. sys/include/timex.h:82: performance (passedByValue): Function parameter 'b' should be passed by reference. sys/include/timex.h:92: performance (passedByValue): Function parameter 'a' should be passed by reference. sys/include/timex.h:92: performance (passedByValue): Function parameter 'b' should be passed by reference. sys/include/timex.h:114: performance (passedByValue): Function parameter 'a' should be passed by reference. sys/include/timex.h:114: performance (passedByValue): Function parameter 'b' should be passed by reference. sys/include/timex.h:147: performance (passedByValue): Function parameter 'a' should be passed by reference. sys/net/gnrc/sock/ip/gnrc_sock_ip.c:153: warning (nullPointer): Possible null pointer dereference: sock - otherwise it is redundant to check it against null. sys/net/gnrc/sock/udp/gnrc_sock_udp.c:191: warning (nullPointer): Possible null pointer dereference: sock - otherwise it is redundant to check it against null.

Running './dist/tools/cppcheck/check.sh master --diff-filter=AC' ✓

The readability issue is due to your static-tests target ;-)

@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Oct 30, 2016

readability issue? the error makes a lot of sense since I accidentally disabled that check (what we talked about above)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 30, 2016

readability issue? the error makes a lot of sense since I accidentally disabled that check (what we talked about above)

The errors are all in one line (its not just the error we talked about ;-))

@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Oct 30, 2016

Oooh okay, I didn't see that, sorry. Thanks, I'll check it out!

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Nov 29, 2016

Is this WIP?

@x3ro x3ro changed the title Provide consistent reason formatting for all cppcheck-suppresses WIP: Provide consistent reason formatting for all cppcheck-suppresses Nov 29, 2016
@miri64 miri64 mentioned this pull request Jan 11, 2017
Merged
@miri64
Copy link
Copy Markdown
Member

miri64 commented Mar 16, 2017

Needs rebase

@miri64
Copy link
Copy Markdown
Member

miri64 commented Apr 25, 2017

Ping?

@miri64
Copy link
Copy Markdown
Member

miri64 commented May 23, 2017

ping?

@aabadie aabadie modified the milestone: Release 2017.07 Jun 26, 2017
@miri64 miri64 mentioned this pull request Jun 27, 2017
Merged
@x3ro x3ro force-pushed the cppcheck-suppress-explained branch 3 times, most recently from 49d66dc to 0a8bb29 Compare October 10, 2017 16:50
@x3ro x3ro changed the title WIP: Provide consistent reason formatting for all cppcheck-suppresses Provide consistent reason formatting for all cppcheck-suppresses Oct 10, 2017
@x3ro x3ro changed the title Provide consistent reason formatting for all cppcheck-suppresses cppcheck: Provide consistent reason formatting for all cppcheck-suppresses Oct 10, 2017
@x3ro x3ro force-pushed the cppcheck-suppress-explained branch from 0a8bb29 to 6998ffb Compare October 10, 2017 17:26
cgundogan
cgundogan approved these changes Oct 10, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

ACK

@x3ro x3ro merged commit f6cd557 into RIOT-OS:master Oct 10, 2017
@miri64 miri64 mentioned this pull request Oct 10, 2017
Closed
@miri64 miri64 mentioned this pull request Nov 28, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

@cgundogan cgundogan cgundogan approved these changes

Assignees

@miri64 miri64

Labels

Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2017.10

Development

Successfully merging this pull request may close these issues.

5 participants

@x3ro @miri64 @OlegHahm @cgundogan @aabadie