BIND 10 #2094: Define and implement RDATA field specs

BIND 10 Development do-not-reply at isc.org
Fri Jul 20 16:56:25 UTC 2012


#2094: Define and implement RDATA field specs
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120731
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:8 vorner]:
 > I pushed two small fixes. One to make it compile for me, another to
 remove an unnecessary whitespace.

 Thanks.  Hmm, my bad if I overlooked a redundant whitespace.

 > Further, I'd like to point to these:
 >
 > The new makefiles contain this. What is the purpose? The In-Memory will
 have nothing in common wit sqlite3, I hope:
 > {{{
 > AM_CPPFLAGS += $(SQLITE_CFLAGS)
 > }}}

 Ah, that's a copy-paste error.  It's not needed, I removed it.
 >
 > In this code, maybe we should at least assert that the `olen` fits into
 `uint8_t`, otherwise we'll silently truncate.

 Hmm, I'd rather make it part of the API contract and if we want to
 assert it, it should better be in the `LabelSequence` class.  But for
 now, I added the assert in the caller side.  (While working on #2091,
 I found we might want to revise the interface for serializing internal
 data of the sequence.  If we actually decide to do it, this point may
 become moot).

 > Why use `FAIL()` and condition, if there could be `ASSERT_EQ()`? The
 `ASSERT_*` macros do allow `<<` too. Also, is it better to have hard-
 failure than `EXPECT_*`?
 > {{{#!c++
 > if (expected_len != actual_len) {
 >     FAIL() << "Wire data mismatch in length:\n"
 >            << "  Actual: " << actual_len << "\n"
 >            << "Expected: " << expected_len << "\n";
 > }
 > }}}

 Ah, yes, XXX_EQ is better.  FAIL() is used simply because the original
 code in libdns++ used it.  I don't remember why we (I) used FAIL() at
 that time, maybe I simply didn't realize we could use XXX_EQ + <<.

 > Is there a reason to start using at-signs instead of backslashes for
 doxygen? It breaks consistency.
 > {{{#!c++
 > /// @file wiredata.h
 > /// @short Utilities for tests with wire data.
 > }}}

 No specific reason, beyond "to be consistent" with other files of
 the directory:-) I actually wondered why these files use @.  So I
 modified all of them using a backslash (I also changed to 'short' to
 'brief' as the latter seems to be used more commonly in our code).

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


More information about the bind10-tickets mailing list