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
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 muks]:
 > I follow now what you mean about allocation (on the heap). I agree that
 the single `uint32_t` makes the copy cheap in this case. I want to check
 the performance overhead of `boost::optional` vs. the `new` and if it
 matters in the context of the current code, but when I have time to spend
 on it.

 > Please review the code in the branch as it uses `boost::optional` like
 in the `RRTTL` code.

 Okay, this is the review result:

 '''changelog'''

 Do we need an entry?

 '''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)
 - 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.

 '''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.

 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.

 '''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)
 - 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_);
 }}}

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