BIND 10 #613: Address CppCheck issues

[BIND 10 Development]

#613: Address CppCheck issues
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:  major    |            Milestone:  A-Team-
                Component:           |  Sprint-20110309
  Unclassified                       |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  3.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 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.

-- 
Ticket URL: <http://bind10.isc.org/ticket/613#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development