BIND 10 #1113: RR type implementation: MINFO

[BIND 10 Development]

#1113: RR type implementation: MINFO
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:
                   Priority:  minor  |  Sprint-20110816
                  Component:         |            Resolution:
  libdns++                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 zzchen_pku]:

 - this is not correct:
 > > {{{
 > > +    /// We use the default copy constructor and assignment operator.
 > > }}}
 > >   Because the copy constructor is actually defined explicitly.  Even
 > >   if it's true the position of the documentation doesn't seem to be
 > >   correct (did you check the doxygen output?).  [...]
 > >   Further, these are not tested in the unittests, so the discrepancy
 > >   isn't properly caught.

 The copy constructor still isn't tested.  Also, when we want to define
 the copy constructor we should normally also define the assignment
 operator.  Why don't you omit the latter?

 > > '''unittest'''
 > > - now that we've merged #904, I strongly suggest extending
 gen_wiredata
 > >   (in the latest master it's in lib/util/python) to support MINFO and
 > >   using it for generating wire-format test data.  hardcoding all hex
 > >   like:

 > Done, added MINFO class and generated testdata file.

 The MINFO class in gen_wiredata looks good, but there are other things
 to do:
 - .wire shouldn't be in EXTRA_DIST (or in the repo).  They are auto
   generated.
 - some test data are still hardcoded.  Although name compression isn't
   very easy to configure in gen-wiredata (because you need to
   calculate the offset), it should still be much more intuitive than
   hardcoding everything.

 > > - not a big deal, but there's inconsistency between rmailbox vs
 > >   rmailbx (with or without 'o') in comment lines and test strings.
 > >   same for emailbox.
 > Okay:) updated.

 There are still some 'bx'.

 > > - I'd avoid using namespace level statically-initialized objects:
 > Updated.

 There's still some:
 {{{
 const generic::MINFO rdata_minfo((string(minfo_txt)));
 }}}
 for safety, it should go to the Rdata_MINFO_Test class.  Also, why did
 you remove rdata_minfo2?

 > > - In createFromWire test, it's not obvious why we use test data file
 named
 > >   "cname" for testing MINFO:
 > > {{{
 > >     EXPECT_THROW(rdataFactoryFromFile(RRType("MINFO"), RRClass("IN"),
 > >                                       "rdata_cname_fromWire", 48),
 > > }}}
 > >   we need a comment about why it's here.

 I'd use gen_wiredata for other cases, and for the last case reuse the
 cname test data with a comment explaining why.

 Other things in the tests:

 - using both RRType::MINFO() and RRType("MINFO") is good, but please
   leave a comment about why you explicitly use the latter form.
   Without any explanation the mixture would look just awkward.
 - in toWireBuffer, why did you remove one of the two tests?
 - in toWireBuffer: rdata_minfo_toWireUncompressed.wire can be
   generated without the rdlen field by specifying a negative value for
   'rdlen' in the spec file, then (I think) the test code will be much
   simpler.
 - same for toWireRenderer.  Also, this should use gen_wiredata.

 > > '''other'''
 > > As we talked at the biweekly call, please consider picking up review
 > > before proceeding to another RDATA task (you seem to have picked up
 > > #1114 while, e.g, #1067 was in the review queue at that time)
 > Okay, I'll consider it next time.

 If you find you cannot take on open review requests for some reason,
 it may be okay to move to another development task.  Sometimes it may
 be because the code to be reviewed is too immature or too big, in
 which case we should rather push it back to the author.  But even in
 such a case, please don't silently go to another development task, but
 do leave some comments in the ticket about the request ticket noting
 why you think you cannot work on that and also raise that at the daily
 call.

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