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