Opened 9 years ago

Closed 9 years ago

#613 closed task (complete)

Address CppCheck issues

Reported by: stephen Owned by: jinmei
Priority: medium Milestone: A-Team-Sprint-20110309
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Have a look at the issues raised by CPP Check

Subtickets

Change History (12)

comment:1 Changed 9 years ago by jreed

It had been discussed months ago that some of the warnings could be ignored. So we should also document those issues and maybe consider tuning the code and/or the cppcheck script to not report them.

comment:2 Changed 9 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:3 Changed 9 years ago by jinmei

Branch trac613 is ready for review.

I've installed cppcheck 1.47 on my laptop, run it, and fixed most of
the trivial issues. I've also added a rule 'cppcheck' to the top
Makefile.am with a suppression file, which contains a list of (semi)
false positives. On this branch 'make cppcheck' should succeed without
any complaints and exit with a code of 0.

I found cppcheck installed on bind10.isc.org was old and it produced
a lot more complaints than 1.47. Some are actually real bugs (e.g. it
correctly detects the bug described in #460). I don't know why 1.47
doesn't complain about them; maybe newer versions tries to err on the
side of fewer false positives.

Anyway, my suggestion is

  • get this branch reviewed and merged
  • run it on some of buildbot boxes regularly. if it stopped we should consider it a showstopper (just like any other build/test failure)

As noted above, it doesn't work well with the currently installed version
of cppcheck on bind10.isc.org. I've built 1.47 under
~jinmei/src/cppcheck-1.47. We can either (copy and) use it or if
possible upgrade the installed version.

This is the proposed changelog entry:

  179.	[func]		jinmei
	Support cppcheck for static code check on C++ code.  If cppcheck
	is available, 'make cppcheck' on the top source directory will run
	the checker and should cleanly complete with an exit code of 0.
	Note: the suppression list isn't included in the final
	distributions.  It should be created by hand or retrieved from
	the git repository.
	(Trac #613, git TBD)

comment:4 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 9 years ago by jinmei

  • Estimated Difficulty changed from 0.0 to 3.0

comment:6 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

May I ask for missingInclude to be included in the suppress list? For some reason, my cppcheck is unable to find all kinds of system libraries (even when the compiler finds them) and spams the output with uninteresting complains.

Also, I got this output:

src/bin/auth/main.cc:185: check_fail: Variable 'dns_service' is allocated memory that is never used (style,unusedVariable)
src/lib/asiolink/tests/io_service_unittest.cc:62: check_fail: Variable 'dns_service' is allocated memory that is never used (style,unusedVariable)
src/lib/asiolink/tests/io_service_unittest.cc:73: check_fail: Variable 'dns_service' is allocated memory that is never used (style,unusedVariable)
src/lib/asiolink/tests/io_service_unittest.cc:84: check_fail: Variable 'dns_service' is allocated memory that is never used (style,unusedVariable)
src/lib/asiolink/tests/io_service_unittest.cc:95: check_fail: Variable 'dns_service' is allocated memory that is never used (style,unusedVariable)
src/lib/asiolink/tests/io_service_unittest.cc:105: check_fail: Variable 'dns_service' is allocated memory that is never used (style,unusedVariable)
src/lib/dns/tests/name_unittest.cc:293: check_fail: Redundant assignment of "copy" to itself (warning,selfAssignment)
src/lib/dns/tests/tsigkey_unittest.cc:105: check_fail: Redundant assignment of "copy" to itself (warning,selfAssignment)

The dns_service ones are most probably false alarms.

From the point of changes, I don't like this much:

-    copy = copy;
-    EXPECT_EQ(0, copy.compare(rdata_unknown));
+    // Self assignment (via a reference to silence cppcheck)
+    generic::Generic& copyref(copy);
+    copyref = copy;
+    EXPECT_EQ(0, copyref.compare(rdata_unknown));

We shouldn't make our code harder to read just to make some tool whose purpose is to help writing better code silent.

Also, why are some local includes (from local directory) changed to search-list ones, even when the header resides in the same directory? That might turn out to be ambiguous and some local includes are left anyway, so it's not because of consistency either.

Also, what is the benefit of using a vector instead of constant-size array as a buffer which size doesn't change?

Thanks.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by jinmei

Replying to vorner:

May I ask for missingInclude to be included in the suppress list? For some reason, my cppcheck is unable to find all kinds of system libraries (even when the compiler finds them) and spams the output with uninteresting complains.

Done.

Also, I got this output:

[snip]

The dns_service ones are most probably false alarms.

As we discussed on jabber, it seems to be due to differences between
cppcheck 1.46 and 1.47. It would be better to ensure clean result with
various versions, but it would also be better to keep the suppress list
shorter. Since in this case the noisy lines are small and it won't be
triggered on our target buildbot box, I'd leave the false alarms on
dns_service open. For redundant assignments, see below.

From the point of changes, I don't like this much:

-    copy = copy;
-    EXPECT_EQ(0, copy.compare(rdata_unknown));
+    // Self assignment (via a reference to silence cppcheck)
+    generic::Generic& copyref(copy);
+    copyref = copy;
+    EXPECT_EQ(0, copyref.compare(rdata_unknown));

We shouldn't make our code harder to read just to make some tool whose purpose is to help writing better code silent.

Actually, I didn't like that much either. My self excuse at that time
was it's only for tests, but now another person complains about it, I've
canceled the change, and exclude the self assignment warnings in the
suppress list.

Also, why are some local includes (from local directory) changed to search-list ones, even when the header resides in the same directory? That might turn out to be ambiguous and some local includes are left anyway, so it's not because of consistency either.

Honestly I was not sure why cppcheck complained only about these. But
in any case now that we've excluded missingInclude, I've simply
canceled the change.

Also, what is the benefit of using a vector instead of constant-size array as a buffer which size doesn't change?

Nothing except it can be initialized in the member initialization list
(or in this case implicit initialization should also work). Since the
warning on initialization in this case is mostly a false positive
(there doesn't seem to be a place where the buffer is used before
set), another option is to exclude this case in the suppress list.
But I thought changing the code was still better than suppressing the
warning because the changed code looked as readable as the original
one to me. Also, since this is only for tests, overheads with
std::vector shouldn't be a problem.

Later in my work on this branch, however, I also realized I could
address the initialization warning by explicitly setting the variable
in the body of the constructor (just like I did in commit 0919193).
So, if you don't like vector, I'm okay with addressing this part that
way.

comment:9 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner
  • Type changed from defect to task

comment:10 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

The dns_service ones are most probably false alarms.

As we discussed on jabber, it seems to be due to differences between
cppcheck 1.46 and 1.47. It would be better to ensure clean result with
various versions, but it would also be better to keep the suppress list
shorter. Since in this case the noisy lines are small and it won't be
triggered on our target buildbot box, I'd leave the false alarms on
dns_service open. For redundant assignments, see below.

OK

Also, what is the benefit of using a vector instead of constant-size array as a buffer which size doesn't change?

Nothing except it can be initialized in the member initialization list
(or in this case implicit initialization should also work). Since the
warning on initialization in this case is mostly a false positive
(there doesn't seem to be a place where the buffer is used before
set), another option is to exclude this case in the suppress list.
But I thought changing the code was still better than suppressing the
warning because the changed code looked as readable as the original
one to me. Also, since this is only for tests, overheads with
std::vector shouldn't be a problem.

I don't really mind it too much, it's just I would never write such code, so I was curious if there's some advantage. I'd trust the compiler in this, it should be simple enough for it to completely optimise it out.

Later in my work on this branch, however, I also realized I could
address the initialization warning by explicitly setting the variable
in the body of the constructor (just like I did in commit 0919193).
So, if you don't like vector, I'm okay with addressing this part that
way.

No need.

It seems OK, merge it, please.

Thanks

comment:11 in reply to: ↑ 10 Changed 9 years ago by jinmei

Replying to vorner:

It seems OK, merge it, please.

Thanks, merged. Closing ticket.

comment:12 Changed 9 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.