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      |
-------------------------------------+-------------------------------------
Changes (by zzchen_pku):

 * owner:  zzchen_pku => jinmei


Comment:

 Replying to [comment:8 jinmei]:
 > 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?
 Added assignment operator.
 As fast as I know, the default copy constructor and assginment operator
 should be enough for MINFO,  but it seems gen-rdatacode.py.in requires all
 rdata types to define their own copy constructor ,don't know the reason.
 > 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.
 Updated, avoid using hardcoding test data.

 > There are still some 'bx'.
 My miss, 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?
 Updated, also added 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.
 Avoid using cname test data.
 > 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.
 Done.
 > - in toWireBuffer, why did you remove one of the two tests?
 Because I thought there is no difference with regard to test  coverage.
 Now I agree a double-test with different data is necessary,  added again.
 > - 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.
 Updated, thanks for your suggestion.
 > 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.
 Okay, good suggestion:)

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