Opened 8 years ago

Closed 8 years ago

#1748 closed task (fixed)

define AbstractRRset::isSameKind() and implement the default version

Reported by: jinmei Owned by: muks
Priority: high Milestone: Sprint-20120320
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: auth performance
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by jinmei)

For #1688, we need to detect whether two RRsets are of the "same
kind", i.e., if the name, RR type, and RR class are the same (in
the context of #1688 other fields and RDATAs would also be expected to
be the same, but the "same kind" itself wouldn't necessarily mean
that).

This condition can be easily implemented, but it would involve
relatively expensive name comparison, and I think we'll like to
optimize that for the specialized in-memory RRset.

So I propose adding a new method to the base (Abstract)RRset class for
the interface of this and provide the straightforward default
implementation.

isSameKind() is a tentative name; it doesn't have to be so if there's
a better name.

// return true iff this and other are of the same kind
bool AbstractRRset::isSameKind(const AbstractRRset& other);

The specialized in-memory version will go to a separate ticket.

This task has no dependency.

Subtickets

Change History (16)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:3 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:4 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 3

comment:5 Changed 8 years ago by jelte

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

comment:6 Changed 8 years ago by muks

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

Picking...

comment:7 Changed 8 years ago by muks

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

Ready for review. Please check in trac1748 branch for the implementation and tests. It also contains work on ticket #1749 as that is based on this bug's work.

comment:8 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:9 follow-ups: Changed 8 years ago by stephen

  • Owner changed from stephen to muks

src/lib/dns/rrset.cc
AbstractRRset::isSameKind():
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
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
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
isSameKind test - can remove the test on rrset_w (it just duplicates the test done on rrset_x).

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

Replying to stephen:

Just happen to notice this via email...

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.)

I'd compare the types first, because comparing names is more
expensive. I think comparing the classes last makes sense. I'd also
make an explicit comment about why we compare these in that specific
order.

comment:11 in reply to: ↑ 9 Changed 8 years ago by muks

Replying to stephen:

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());
}

Changed the code to look like this now.

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

Done :)

src/lib/dns/tests/rrset_unittest.cc
I would add one check: that an RRset is the same kind as itself, i.e. add

EXPECT_TRUE(rrset_w.isSameKind(rrset_w));

Done :)

src/lib/datasrc/rbnode_rrset.h
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.

Done :)

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.

Added :)

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

I've changed the test to check a different name now.

comment:12 in reply to: ↑ 10 Changed 8 years ago by muks

Replying to jinmei:

I'd compare the types first, because comparing names is more
expensive. I think comparing the classes last makes sense. I'd also
make an explicit comment about why we compare these in that specific
order.

The original code compared type, class and name in that order. I still think that's the proper order as the class comparison is O(1) and name should come last. But I've changed the code and put in the comment for type, name, class in that order.

comment:13 Changed 8 years ago by muks

  • Owner changed from muks to stephen

Ticket back to stephen for review :)

comment:14 Changed 8 years ago by stephen

  • Owner changed from stephen to muks

Reviewed commit a8965bdfe892d5aee1edddaaaad7d67cda7ea1f0

All OK, please merge.

comment:15 Changed 8 years ago by muks

Pushed to master:

* ac2c63f bug #1749: Address review comments (from bug #1748)
* 02db775 bug #1748: Add one more testcase comparing a RRset with itself
* 1007c99 bug #1748: Change code to use a condition expression
* 216b2e1 [1749] Implement a specialized RBNodeRRset::isSameKind()
* 64eef65 [1748] Define AbstractRRset::isSameKind() and implement the default version

comment:16 Changed 8 years ago by muks

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

Resolving as fixed.

Note: See TracTickets for help on using tickets.