BIND 10 #1748: define AbstractRRset::isSameKind() and implement the default version

[BIND 10 Development]

#1748: define AbstractRRset::isSameKind() and implement the default version
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  high   |  Sprint-20120320
                  Component:         |            Resolution:
  libdns++                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  3
Feature Depending on Ticket:  auth   |           Total Hours:  0
  performance                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => muks


Comment:

 '''src/lib/dns/rrset.cc'''[[BR]]
 AbstractRRset::isSameKind():[[BR]]
 A couple of points with the existing code:
 * Even single-line "if" statements should include braces.
 * Value returned with "return" should be enclosed in brackets.

 However, since the return value is boolean, and since each comparison
 generates a boolean value that can be combined with the other values via
 logical operators, I suggest that the following is clearer:
 {{{
 bool
 AbstractRRset::isSameKind(const AbstractRRset& other) const {
     return (getName() == other.getName() &&
             getType() == other.getType() &&
             getClass() == other.getClass());
 }
 }}}
 (As the expressions are evaluated left to right and not evaluated if the
 result of the expression becomes known during earlier evaluations, it will
 be more efficient to check "class" last.  Most of the time everything is
 the same class, so the name or type not matching is more probable than the
 class not matching.)

 isSameKind() can be declared const as it doesn't change anything in the
 class, and the methods it calls are const.

 '''src/lib/dns/tests/rrset_unittest.cc'''[[BR]]
 I would add one check: that an RRset is the same kind as itself, i.e. add
 {{{
 EXPECT_TRUE(rrset_w.isSameKind(rrset_w));
 }}}
 There is no reason it should not be, but on the other had it is always
 possible - however unlikely - that that in some implementations,
 isSameKind() internally modifies the state of an RRset (even with the
 "const" qualifier).

 '''src/lib/datasrc/rbnode_rrset.h'''[[BR]]
 RBNodeRRset::isSameKind():
 * "rb" can be declared and initialized in the same line.
 * Is there a need for "t" - can't "&other" be put as the argument to
 dynamic_cast?
 * As "rb" is not "bool", I would prefer the check "rb != NULL".
 * Even single-line "if" statements should include braces.
 * Value returned with "return" should be enclosed in brackets.

 The code is an optimisation for comparing RBNodeRRsets.  However, in doing
 this optimisation, semantically the code is not "is same kind" but is
 instead "is identical object".  I think it deserves a comment explaining
 why there is this difference.


 '''src/lib/datasrc/tests/rbnode_rrset_unittest.cc'''[[BR]]
 isSameKind test - can remove the test on rrset_w (it just duplicates the
 test done on rrset_x).

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