Opened 6 years ago

Closed 6 years ago

#3439 closed defect (fixed)

cppcheck fails after the merge of #3409.

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea0.9
Component: configuration Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

After the merge of #3409 the cppcheck started to complain about the duplicated check in one of the functions:

http://git.kea.isc.org/~tester/builder/KEA-cppcheck/20140429204501-FreeBSD8-amd64-GCC/logs/cppcheck.out

The fix in the unit test is:

	

    From af16ec45a0f6ce53ae5809a4daae49ac3fdbad6a Mon Sep 17 00:00:00 2001
    From: Marcin Siodelski <marcin@isc.org>
    Date: Wed, 30 Apr 2014 21:07:05 +0200
    Subject: [PATCH] [master] Fixed error in the dhcp_parsers_unittest.cc
     
    Invalid two variables were compared.
    ---
    src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc | 4 ++--
     1 file changed, 2 insertions(+), 2 deletions(-)
     
    diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
    index deb41cc..ce1961b 100644
    --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
    +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
    @@ -1126,8 +1126,8 @@ public:
                          values->getPosition("foo").line_) ||
                         (ref_values->getPosition("foo").pos_ !=
                          values->getPosition("foo").pos_) ||
    -                    (ref_values->getPosition("foo").pos_ !=
    -                     values->getPosition("foo").pos_));
    +                    (ref_values->getPosition("foo").file_ !=
    +                     values->getPosition("foo").file_));
         }
     
         /// @brief Check that option definition storage in the context holds
    -- 
    1.9.0

However, the fact that the function was broken and none of the unit tests failed indicates that there is some untested permutation. In addition to this patch, there should be some test case added to cover that.

Subtickets

Change History (8)

comment:1 Changed 6 years ago by tomek

  • Milestone changed from Kea-proposed to Kea0.9

comment:2 Changed 6 years ago by marcin

  • Owner changed from tomek to marcin
  • Status changed from new to accepted

comment:3 Changed 6 years ago by marcin

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

This ticket is now ready for a review. The bug in the unit test has been corrected and the unit tests have been extended to check more thoroughly check that the position of the configuration parameter hasn't changed when the original context was changed.

I don't think it requires a ChangeLog entry as it is the user-invisible change and it doesn't change the behavior of the parser(s).

comment:4 Changed 6 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 6 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 643f0f1b04d30a85a56970795de210557c291ca4

Correction and additional tests seem OK. However...

In the class ParserContextTest, the check{Value|Position}{Eq|Neq} functions take two template arguments, ContainerType and ValueType. The ValueType argument is not needed - it not referenced anywhere. And if we get rid of ValueType template argument, the calls to the functions do not need to specify the template argument as it can be inferred from the type of the method arguments, i.e. a call like:

checkPositionNeq<BooleanStorage, bool>("pos0", ...)

can be replaced with

checkPositionNeq("pos0", ...)

Agree that this change does not need a ChangeLog entry.

comment:6 in reply to: ↑ 5 Changed 6 years ago by marcin

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit 643f0f1b04d30a85a56970795de210557c291ca4

Correction and additional tests seem OK. However...

In the class ParserContextTest, the check{Value|Position}{Eq|Neq} functions take two template arguments, ContainerType and ValueType. The ValueType argument is not needed - it not referenced anywhere. And if we get rid of ValueType template argument, the calls to the functions do not need to specify the template argument as it can be inferred from the type of the method arguments, i.e. a call like:

checkPositionNeq<BooleanStorage, bool>("pos0", ...)

can be replaced with

checkPositionNeq("pos0", ...)

Yes, blindly copying an existing code as a base to a new code is almost never good. I removed the unused parameter from the new and old functions that had it.

Agree that this change does not need a ChangeLog entry.

comment:7 Changed 6 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 79df55092fca9d6528fa1c265aa3b68647ed98c5

All OK, please merge.

comment:8 Changed 6 years ago by marcin

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

Merged with commit fccc9c599e2e38ef8e21b67e5e593fa4ac832696

Note: See TracTickets for help on using tickets.