Opened 9 years ago

Closed 9 years ago

#16 closed enhancement (fixed)

review DNS Name class (and some related minor classes)

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone:
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

The initial set of implementation, documentation, and test cases is completed and ready for review. It's available at svc branch:
svn(+ssh):bind10.isc.org/svn/bind10/branches/jinmei-dnsmessageapi

Subtickets

Change History (9)

comment:1 Changed 9 years ago by jelte

  • Owner changed from jinmei to jelte
  • Status changed from new to assigned
  • Summary changed from DNS Name class (and some related minor classes) to review DNS Name class (and some related minor classes)

comment:2 Changed 9 years ago by jelte

  • Status changed from assigned to accepted

comment:3 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei
  • Status changed from accepted to assigned

Simply including my file with notes here, if things here are too brief, let me know. Overall: code looking very well, documentation more than adequate afaict.

general:

is there a specific reason for AC_PREREQ([2.64])?

should we use the throw statement for exceptions (mostly so
they automatically end up in doxygen)

headerfile tabbing; start at start of line within namespace, or 1 tab?

---

doc nits:

getLength -> s/length/number of octets/ (or perhaps length
(i.e. the number of octets of the wire format) or something

---

API nits:

getLabels() -> getLabelCount? or is that too long?

TooLongName?->NameTooLong??
TooLongLabel?->LabelTooLong??

question: should we make OutputBuffer? a base class?
and add an option in renderer to, for instance, render to representation
format in ascii?

comparisonresult: i think we shouldn't consider the root label a
common ancestor (unless you know of a use-case for it)

add operator+ for Name objects (concatenate)?
should we also add an Name::append(const Name& suffix)?

---

Source comments:

name.cc:108: Add a todo to re-evaluate the number here?
I have a feeling it could be a lot smaller

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

  • Status changed from assigned to accepted

Answering some selected points:

Simply including my file with notes here, if things here are too
brief, let me know. Overall: code looking very well, documentation
more than adequate afaict.

Thanks for the review!

general:

is there a specific reason for AC_PREREQ([2.64])?

No, I guess it was autogenerated by some of the autotools I'm using.
I believe it can be much older versions. Do you have a suggestion?

should we use the throw statement for exceptions (mostly so
they automatically end up in doxygen)

Instead of dns_throw()? I hate macros, so I'm okay with that. The
only concern (and the only reason why I introduced a macro here) is it
would make the throw lines longer with boring arguments (FILE and
LINE).

Another point about exceptions. Maybe we should make
isc::dns::Exception a derived class of std::exception?

headerfile tabbing; start at start of line within namespace, or 1 tab?

Do you mean
namespace isc {

namespace dns {

class Foo {

instead of this one?
namespace isc {
namespace dns {
class Foo {

I can live with either style. The reason why I chose the latter is
that the former style will add meaningless space to the left,
requesting more multi-line statements if we keep line columns
reasonably short.

And, actually, I'd rather replace "namespace XXX {" with a convenience
macro, like:
START_ISC_NAMESPACE

because the latter style above has its own drawback: it's not friendly
with some Emacs commands (e.g. c-beginning-of-defun doesn't work
correctly).

getLabels() -> getLabelCount? or is that too long?

I tend to agree, as getLabels() may sound like returning a set of
labels.

name.cc:108: Add a todo to re-evaluate the number here?
I have a feeling it could be a lot smaller

Do you mean this part of "constructor-from-string"?

offsets.reserve(Name::MAX_LABELS);

this is a local placeholder with a possible maximum space to avoid
reallocation. The resulting offset vector will have the exact size:

offsets_.assign(offsets.begin(), offsets.end());

Doesn't it make sense, or are you talking about something different?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by jelte

Replying to jinmei:

general:

is there a specific reason for AC_PREREQ([2.64])?

No, I guess it was autogenerated by some of the autotools I'm using.
I believe it can be much older versions. Do you have a suggestion?

i'm guessing 2.59 would be a nice base line, but we might want to look around a bit

should we use the throw statement for exceptions (mostly so
they automatically end up in doxygen)

Instead of dns_throw()? I hate macros, so I'm okay with that. The
only concern (and the only reason why I introduced a macro here) is it
would make the throw lines longer with boring arguments (FILE and
LINE).

actually what i meant was use them in the function signature;
void isc::example::Class::myFunction(int arg) throw (isc::example::ExampleException?);

don't know if there are any static checkers, but those would be nice. One thing this does do is add them automatically to doxygen output, so we don't have to enumerate exceptions in the description (unless we want to explain when they are thrown)

Another point about exceptions. Maybe we should make
isc::dns::Exception a derived class of std::exception?

sounds reasonable

And, actually, I'd rather replace "namespace XXX {" with a convenience
macro, like:
START_ISC_NAMESPACE

because the latter style above has its own drawback: it's not friendly
with some Emacs commands (e.g. c-beginning-of-defun doesn't work
correctly).

let's put that up as a general style discussion item (i'm at this point not a fan of this proposal ;) )

name.cc:108: Add a todo to re-evaluate the number here?
I have a feeling it could be a lot smaller

Do you mean this part of "constructor-from-string"?

offsets.reserve(Name::MAX_LABELS);

this is a local placeholder with a possible maximum space to avoid
reallocation. The resulting offset vector will have the exact size:

offsets_.assign(offsets.begin(), offsets.end());

Doesn't it make sense, or are you talking about something different?

well this makes it more clear, but i'm not certain if this is the more efficient choice (always reserve full amount vs. reallocation for big ones, how many 'big ones' (say more than 10 labels) do we expect? or perhaps 32 for ip6.arpa leaves?) though i might already be scratching for bytes here :)

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

Replying to jelte:

Below, I'm trying to answer the questions. Although there are some
open questions, I guess we can merge the current branch to the main
trunk at this point and discuss the open issues for incremental
updates.

should we use the throw statement for exceptions (mostly so
they automatically end up in doxygen)

Instead of dns_throw()? I hate macros, so I'm okay with that. The
only concern (and the only reason why I introduced a macro here) is it
would make the throw lines longer with boring arguments (FILE and
LINE).

actually what i meant was use them in the function signature;
void isc::example::Class::myFunction(int arg) throw (isc::example::ExampleException?);

don't know if there are any static checkers, but those would be
nice. One thing this does do is add them automatically to doxygen
output, so we don't have to enumerate exceptions in the description
(unless we want to explain when they are thrown)

Ah, okay, so you meant exception specifications by "throw statement".

Actually, I (implicitly) intentionally avoided using them in my
implementation based on suggestions from "More Effective C++" and "C++
Coding Standards". It seemed to me exception specifications have too
many drawbacks and their advantage doesn't justify the troubles. To
name a few:

  • specification violations are basically only detected run-time, and the default behavior for violation is abort.
  • exception specifications apply recursively. so, in the above example, we need to examine all functions that can be called from myFunction() to provide a reasonable specification.
  • the second point also applies to standard libraries, and so we need to be careful (especially) about std::bad_alloc everywhere in our code. I suspect this is what we want to avoid in our use of exceptions.
  • adding an exception specification essentially means we add try-catch in the function. so it also has performance penalty (though perhaps slightly).

How to enumerate possible exceptions in the document is certainly an
issue. Maybe we should solve this problem by introducing a fixed
custom format of doxygen so that we can check them relatively easily
and as a formal process.

Another point about exceptions. Maybe we should make
isc::dns::Exception a derived class of std::exception?

sounds reasonable

Done. I'm afraid the change is non-trivial, so it would be nice if
you can perform sanity checks. It's rev 421.

And, actually, I'd rather replace "namespace XXX {" with a convenience
macro, like:
START_ISC_NAMESPACE

because the latter style above has its own drawback: it's not friendly
with some Emacs commands (e.g. c-beginning-of-defun doesn't work
correctly).

let's put that up as a general style discussion item (i'm at this
point not a fan of this proposal ;) )

Okay, let's discuss this separately.

name.cc:108: Add a todo to re-evaluate the number here?
I have a feeling it could be a lot smaller

Do you mean this part of "constructor-from-string"?

(I meant constructor-from-wire)

offsets.reserve(Name::MAX_LABELS);

this is a local placeholder with a possible maximum space to avoid
reallocation. The resulting offset vector will have the exact size:

offsets_.assign(offsets.begin(), offsets.end());

Doesn't it make sense, or are you talking about something different?

well this makes it more clear, but i'm not certain if this is the
more efficient choice (always reserve full amount vs. reallocation
for big ones, how many 'big ones' (say more than 10 labels) do we
expect? or perhaps 32 for ip6.arpa leaves?) though i might already
be scratching for bytes here :)

I'm not certain either, but since the full amount in this case is 128
bytes, my gut feeling is that the cost of allocating "full" isn't so
different from that of some "reasonable smaller default," e.g, 32
bytes. And, in either case, I suspect the difference doesn't affect
overall performance much - if we want to be sure we could perform
micro benchmarks later.

Now I'm going to answer the rest of initial comments:

TooLongName??->NameTooLong???
TooLongLabel??->LabelTooLong???

I don't have a strong opinion. These are basically a matter of
preference, right? If you have other reasons for preferring the
latter or if others prefer the latter, I'll make the change;
otherwise, let's just keep the current naming.

question: should we make OutputBuffer?? a base class?
and add an option in renderer to, for instance, render to representation
format in ascii?

I've added the current design choice that I explained on jabber in
buffer.h. Right now, I think we can/should go with the current
design. If necessary we'll revisit it.

Regarding "render in ascii", however, I'm not sure what you mean...do
you mean something like

076578616d706c6500 (for "example.")?

or do you mean textual representation of a DNS message, like as dig
output? If it's the latter, I guess we'd directly implement in the
"message" class.

In any case, if we see real need for this, I believe we can easily
extend the API without modifying the existing interfaces.

comparisonresult: i think we shouldn't consider the root label a
common ancestor (unless you know of a use-case for it)

I see the point, and I don't think the current design is the best,
either. But this was actually an intentional design choice,
concerning how/whether to handle "non absolute" names. I've noted
this point in name.h, and I'd defer the decision later.

add operator+ for Name objects (concatenate)?

I don't have a strong opinion, but I incline to not to use operator
overloading here, due to its drawbacks (see google's coding guideline
on this point)
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

IMO, the equality operator (and its friends) are worth having in many
cases, and some of them are crucial in specific usage (like operator<
in STL containers). For other operators, I'd avoid them unless we
have a strong compelling reason for having it.

should we also add an Name::append(const Name& suffix)?

Perhaps, but I'd defer the decision until we implement other part of
our package. I'd begin with a minimum set of interfaces and add
extensions as we see the need for them.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by jelte

  • Status changed from accepted to assigned

Replying to jinmei:

Replying to jelte:

Below, I'm trying to answer the questions. Although there are some
open questions, I guess we can merge the current branch to the main
trunk at this point and discuss the open issues for incremental
updates.

i agree, i think we kind of reverted on the decision to let the reviewer do the merging (since then the subversion credits would go to the wrong person), so you may do the honor. Should we put a list of open 'issues' to the list or bring them up in the next meeting?

Ah, okay, so you meant exception specifications by "throw statement".

  • specification violations are basically only detected run-time, and the default behavior for violation is abort.
  • exception specifications apply recursively. so, in the above example, we need to examine all functions that can be called from myFunction() to provide a reasonable specification.

imo, that's a feature, not a bug :)

  • the second point also applies to standard libraries, and so we need to be careful (especially) about std::bad_alloc everywhere in our code. I suspect this is what we want to avoid in our use of exceptions.
  • adding an exception specification essentially means we add try-catch in the function. so it also has performance penalty (though perhaps slightly).

hmm, does it? that's a pretty good reason to not do it.

How to enumerate possible exceptions in the document is certainly an
issue. Maybe we should solve this problem by introducing a fixed
custom format of doxygen so that we can check them relatively easily
and as a formal process.

yeah, i think we should still add the 'recursives' though. And try to find a way to statically check them. I miss the Java way of having runtime exceptions and non-runtime exceptions (where the latter have to be caught or you get a compile error). And i've already run into problems because of uncaught exceptions in the parkinglot a few times now.

Another point about exceptions. Maybe we should make
isc::dns::Exception a derived class of std::exception?

sounds reasonable

Done. I'm afraid the change is non-trivial, so it would be nice if
you can perform sanity checks. It's rev 421.

looks ok. Although i am now wondering whether it should be isc::Exception instead of isc::dns::Exception :)

well this makes it more clear, but i'm not certain if this is the
more efficient choice (always reserve full amount vs. reallocation
for big ones, how many 'big ones' (say more than 10 labels) do we
expect? or perhaps 32 for ip6.arpa leaves?) though i might already
be scratching for bytes here :)

I'm not certain either, but since the full amount in this case is 128
bytes, my gut feeling is that the cost of allocating "full" isn't so
different from that of some "reasonable smaller default," e.g, 32
bytes. And, in either case, I suspect the difference doesn't affect
overall performance much - if we want to be sure we could perform
micro benchmarks later.

ack

TooLongName??->NameTooLong???
TooLongLabel??->LabelTooLong???

I don't have a strong opinion. These are basically a matter of
preference, right? If you have other reasons for preferring the
latter or if others prefer the latter, I'll make the change;
otherwise, let's just keep the current naming.

my reason is that the current name 'sounds wrong', but please ask around

or do you mean textual representation of a DNS message, like as dig
output? If it's the latter, I guess we'd directly implement in the
"message" class.

ah, ok.

See top, i think it is ready for merge.

comment:8 in reply to: ↑ 7 Changed 9 years ago by jinmei

Replying to jelte:

  • the second point also applies to standard libraries, and so we need to be careful (especially) about std::bad_alloc everywhere in our code. I suspect this is what we want to avoid in our use of exceptions.
  • adding an exception specification essentially means we add try-catch in the function. so it also has performance penalty (though perhaps slightly).

hmm, does it? that's a pretty good reason to not do it.

which one of "it"? :-) if it's the latter, it should be quite obvious
because otherwise the generated code wouldn't be able to detect
violation run-time. To be very sure, try compiling the following code
enabling/disabling WITH_xxx definitions with g++ -S and see the
generated code. (Note: the actual run time cost depends on the
compiler implementation - g++ with DWARF2-based exception handling
doesn't have overhead unless an exception is actually thrown)

#include <stdexcept>

extern void bar();

enable either of them for experiments
#define WITH_EXCEPTION_SPEC 1
#define WITH_TRYCATCH 1

#ifdef WITH_EXCEPTION_SPEC
void
foo(int x) throw() {

bar();
return;

}
#elif defined(WITH_TRYCATCH)
void
foo(int x) {

try {

bar();

} catch (...) {}
return;

}
#else
void
foo(int x) {

bar();
return;

}
#endif

comment:9 Changed 9 years ago by jinmei

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

Merged to trunk. Open issues (for revisiting them later) are moved to a separate ticket. Closing.

Note: See TracTickets for help on using tickets.