Opened 9 years ago

Closed 9 years ago

#21 closed enhancement (fixed)

review RRtype/class and TTL 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 jinmei-dnsrrparams branch is ready for review.

To see the diff, use svn diff with the trunk:
% reptop=svn+ssh://bind10.isc.org/svn/bind10
% svn diff $reptop/trunk $reptop/branches/jinmei-dnsrrparams

I've updated some of the files that existed when I created this
branch, but the major changes are in new files:
rrtype.{h,cc}
rrclass.{h,cc}
rrttl.{h,cc}
rrparamregistry.{h,cc}
and corresponding _unittest.cc's.

The implementation should be pretty straightforward and easy to review
(I believe). The only non trivial part would be some tricks used in
the rrparamregistry implementation.

doxygen-generated HTML documents are available at:
http://bind10.isc.org/~jinmei/bind10/cpp/

Subtickets

Change History (3)

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

  • Status changed from new to assigned

All looking fine, one question, one minor thing, and two cosmetic things;

Question; can we still use types/classes directly? (if something.getType() == RRType::A)?

I'm a little worried about threads (should someone ever use those), and if that won't fail if they start modifying the reg. Maybe this is not a problem, and if it is, I guess we could simply add to the documentation "do reg modifications before spawning threads".

Cosmetic: I'd rename isc::dns::RRTypeExist to isc::dns::RRTypeExists and isc::dns::RRClassExist to isc::dns::RRClassExists

comment:2 in reply to: ↑ 1 Changed 9 years ago by jinmei

Replying to jelte:

All looking fine, one question, one minor thing, and two cosmetic things;

Thanks for the review!

Question; can we still use types/classes directly? (if
something.getType() == RRType::A)?

Do you mean instead of RRType::A()? If so, no, we can't, because, as
documented, such a non-local static object cannot (always) be safely
used to initialize other non-local static objects.

We could add the variable version of RRType::A() with a note that it
shouldn't be used to initialize non-local static objects, but IMO we
should consider that option only after we know the function version
has a significant overhead (and note that these functions are
intentionally defined as inline).

The function-call style may look awkward, if that's your concern, but
in my understanding this is something we have to live with in C++.

I'm a little worried about threads (should someone ever use those),
and if that won't fail if they start modifying the reg. Maybe this
is not a problem, and if it is, I guess we could simply add to the
documentation "do reg modifications before spawning threads".

Yeah, I was aware of multi-thread implications. In general, any
singleton class has this issue (and in fact the well-known RR
type/class interfaces such as RRType::A() has the same problem). As
you noted, however, I think we can leave it thread-unsafe for now,
because the RR parameter registry would be mostly static in common
usage. If we need to make it thread-safe later, I believe we can do
it without affecting the API.

At the moment, I've added implementation notes on the thread safeness
to the code document.

Cosmetic: I'd rename isc::dns::RRTypeExist to isc::dns::RRTypeExists
and isc::dns::RRClassExist to isc::dns::RRClassExists

Okay.

Since there doesn't seem to be blocking issue, I'm going to merge the
branch and close the ticket.

comment:3 Changed 9 years ago by jinmei

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.