Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2650 closed defect (fixed)

Errors from cppcheck 1.58

Reported by: vorner Owned by: muks
Priority: medium Milestone: Sprint-20130219
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Once again, I upgraded my cppcheck, this time to version 1.58. And, as always, there are new suggestions:

src/lib/asiodns/udp_server.cc:62: check_fail: Member variable 'Data::bytes_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/asiodns/udp_server.cc:78: check_fail: Member variable 'Data::bytes_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/asiodns/udp_server.cc:106: check_fail: Member variable 'Data::bytes_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/zone_finder.cc:552: check_fail: The class 'InMemoryZoneFinder' does not have a constructor. (style,noConstructor)
src/lib/datasrc/memory_datasrc.cc:784: check_fail: The class 'InMemoryZoneFinder' does not have a constructor. (style,noConstructor)
src/lib/datasrc/sqlite3_accessor.cc:684: check_fail: The class 'SQLite3Accessor' does not have a constructor. (style,noConstructor)
src/lib/datasrc/sqlite3_accessor.cc:889: check_fail: The class 'SQLite3Accessor' does not have a constructor. (style,noConstructor)
src/lib/dns/master_loader.cc:57: check_fail: The class 'MasterLoader' does not have a constructor. (style,noConstructor)
src/lib/util/io/socketsession.cc:85: check_fail: Member variable 'ForwarderImpl::fd_' is not initialized in the constructor. (warning,uninitMemberVar)

Subtickets

Change History (16)

comment:1 Changed 7 years ago by jwright

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:2 Changed 7 years ago by shane

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:3 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130219

comment:4 Changed 7 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking

comment:5 in reply to: ↑ description Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

Up for review. Doesn't require a ChangeLog.

The following are not fixed as they are cppcheck false positives. But I want the reviewer to verify these:

src/lib/datasrc/memory/zone_finder.cc:552: check_fail: The class 'InMemoryZoneFinder' does not have a constructor. (style,noConstructor)
src/lib/datasrc/memory_datasrc.cc:784: check_fail: The class 'InMemoryZoneFinder' does not have a constructor. (style,noConstructor)
src/lib/datasrc/sqlite3_accessor.cc:684: check_fail: The class 'SQLite3Accessor' does not have a constructor. (style,noConstructor)
src/lib/datasrc/sqlite3_accessor.cc:889: check_fail: The class 'SQLite3Accessor' does not have a constructor. (style,noConstructor)
src/lib/dns/master_loader.cc:57: check_fail: The class 'MasterLoader' does not have a constructor. (style,noConstructor)

In each of these cases, the class is prefixed by its enclosing class:

class InMemoryZoneFinder::Context : public ZoneFinder::Context {
class InMemoryZoneFinder::Context : public ZoneFinder::Context {
class MasterLoader::MasterLoaderImpl {

cppcheck thinks there's a constructor missing for the enclosing class. Constructors are present for the current class (and the enclosing class too, but that's not at the failure line).

I suggest we ignore these until cppcheck gets fixed (and not suppress them).

comment:6 follow-up: Changed 7 years ago by jelte

  • Owner changed from UnAssigned to muks

The changes look OK.

But I'm not sure whether we shouldn't suppress the false positives; Obviously we should suppress as little as possible, but what is the purpose of the ticket here, to fix any real problems that cppcheck points out or to make sure 'make cppcheck' does not fail on systems that are more up-to-date?

(btw I have checked with the current -dev version; it still has the false positives, and yet more errors)

note that for 1.56 we actuall did add some (global) suppressions. At some point we do need to remove them :)

comment:7 in reply to: ↑ 6 Changed 7 years ago by muks

  • Owner changed from muks to jelte

Replying to jelte:

The changes look OK.

But I'm not sure whether we shouldn't suppress the false positives; Obviously we should suppress as little as possible, but what is the purpose of the ticket here, to fix any real problems that cppcheck points out or to make sure 'make cppcheck' does not fail on systems that are more up-to-date?

The purpose is definitely to check that our code is correct.

If this is indeed a cppcheck regression, this could be something that may get fixed in the next update or two (as it's very visible), so it may be worth waiting rather than adding suppressions.

I tried to make a cppcheck testcase to send to its authors, but it isn't failing for my test code:

class A {
public:
  A(int) {}

  class B;
};

class A::B {
public:
  B(int) {
  }
};

I don't know if its authors would be too happy to go through BIND 10 code to check this.

(btw I have checked with the current -dev version; it still has the false positives, and yet more errors)

note that for 1.56 we actuall did add some (global) suppressions. At some point we do need to remove them :)

Nod. We should keep removing suppressions when possible.

comment:8 Changed 7 years ago by vorner

Well, the reasons for this ticket are two. One is, I got bunch of errors and didn't spend time to examine them, so I create a ticket to allocate time for it in some sprint and we can check the errors are either harmless or fix them.

The other is, when I run cppcheck, if there are too many errors, I miss the new ones. If it doesn't fail usually and suddenly it does, I notice it easier. But I don't know if it's worth an entry in the suppression list.

comment:9 Changed 7 years ago by jelte

  • Owner changed from jelte to muks

i can't reproduce it either.

I did discover that making the private members public makes the false positive go away, perhaps that helps :)

I'm thinking that perhaps we should consider version-specific suppressions (so that at least we can run make cppcheck on our own machines, those of us that have more recent ones, if it always reports errors it is too easy to either not run or overlook regressions)

If we can live with these errors for now, this can be merged. But we should file a bug report if there isn't one already

comment:10 Changed 7 years ago by muks

  • Owner changed from muks to jelte

What shall I do for this bug now? :)

Shall we merge as it is, and wait and see if the next cppcheck upgrade fixes these issues?

comment:11 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to muks

jeremy says suppress :) (if he upgrades the server version we'll need to btw)

comment:12 in reply to: ↑ 11 Changed 7 years ago by muks

  • Owner changed from muks to jelte

Replying to jelte:

jeremy says suppress :) (if he upgrades the server version we'll need to btw)

Suppressions added now. :)

comment:13 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to muks

ok, verified :)

Please go ahead and merge. Did we report a bug yet?

comment:14 in reply to: ↑ 13 Changed 7 years ago by muks

Replying to jelte:

ok, verified :)

Please go ahead and merge. Did we report a bug yet?

Upstream? No, because I don't know how to explain the bug. It seems to be happening in the type of code described above, but there seems to be something else that triggers it.

comment:15 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit ea110815f2864c353ac69ba54dd837987890d496:

* e540ba5 [2650] Add cppcheck suppressions
* db77f1f [2650] Initialize member variable in constructor
* 2284240 [2650] Initialize member variable in constructor
* aeda8e4 [2650] Remove unused member variable

Resolving as fixed. Thank you for the reviews Jelte.

comment:16 Changed 7 years ago by muks

I have reported this ticket upstream:
https://sourceforge.net/apps/trac/cppcheck/ticket/4574

Note: See TracTickets for help on using tickets.