BIND 10 #2565: Refactor RR param parsing code in MasterLoader

[BIND 10 Development]

#2565: Refactor RR param parsing code in MasterLoader
-------------------------------------+-------------------------------------
            Reporter:  muks          |                        Owner:
                Type:  enhancement   |  jinmei
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130108
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:8 jinmei]:
 > '''changelog'''
 >
 > Do we need an entry?

 I don't think we need a `ChangeLog` entry. Even though there are some
 libdns++ changes, these don't seem to modify existing public API.

 > '''rrclass-placeholder.h'''
 >
 > - s/details/detailed/?
 > {{{#!cpp
 >     /// this reason this function intentionally omits the capability of
 >     /// delivering details reason for the parse failure, such as in the
 > }}}
 >   (If so, this was probably an error in the `RRTTL` text which seemed
 >   to be mostly copy-pasted here.  So both should be fixed)

 Both cases are fixed now.

 > - on a related note, the documentation for `RRTTL` and `RRClass` is
 >   essentially the same, just parameterized with the class name.
 >   Ideally they should be unified according to DRY.  But I don't have
 >   a good idea for that right now, so if you don't have one either,
 >   maybe we should live with it for now.

 I don't have any ideas on how to unify documentation as stated too.

 > '''rrparamregistry'''
 >
 > This is actually beyond the scope of this task, but I think it's
 > better to keep the signature/interface of textToXXXCode look
 > consistent for XXX being Type and Code.  So I'd extend textToTypeCode
 > in the same way.

 Done.

 > I'd also note (in the doc) the rationale of the interface, especially
 > about why we simply don't throw and return boolean when it fails to
 > convert the text.

 Done.

 > '''master_loader.cc'''
 >
 > - This seems to be a bit more expensive than ideal:
 > {{{#!cpp
 >             if (rrclass != zone_class_) {
 > }}}
 >   If I understand it correctly, this will construct another optional
 >   object from zone_class_ (using an implicit constructor) and calls
 >   operator!=.  I suspect it's slightly better in performance:
 > {{{#!cpp
 >             if (*rrclass != zone_class_) {
 > }}}
 >   (and, in general, I'd prefer explicit representations even if its
 >   performance advantage is marginal)

 Done.

 > - I guess we should simply throw `InternalException` (and remove the
 >   comment here), just like the previous version:
 > {{{#!cpp
 >                 // It doesn't really matter much what type of exception
 >                 // we throw, we catch it just below.
 >                 isc_throw(isc::BadValue, "Class mismatch: " << rrclass
 <<
 >                           "vs. " << zone_class_);
 > }}}

 Done.

 I've also pushed some other misc. updates.

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