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:10 zzchen_pku]:

 The latest version basically looks fine and ready for merge.

 I have some minor additional comments though.  I don't think we need
 another cycle of review for this, so please update the code
 accordingly and then merge it.

 - Unfortunately this comment is not 100% correct:
 {{{
 +    /// This method never throws an exception.
 +    MINFO& operator=(const MINFO& source);
 }}}
   because coping a name may result in std::bad_alloc.  I personally
   even note this obvious, but others seem to rather ignore this level
   of exceptions in documentation.  Either is okay for me, but at least
   "never throws" is not correct and should be fixed.

 - I'd remove the comment line because it now has MINFO specific code
   in it (although technically it may not be considered
   "specialization"):
 {{{
 +class Rdata_MINFO_Test : public RdataTest {
 +    // there's nothing to specialize
 }}}

 - The assignment test: this is fine, although it's probably too much
   because the implementation is a trivial copy (rather than using
   pimpl which requires a bit more care).  But I'm okay with keeping
   the current test case.

 - rdata_minfo_toWireUncompressed2.spec: the comment doesn't seem to be
   correct (it customizes the rmailbox).  Ideally it describes the
   point of the test correctly.  Or at least the incorrect part of the
   comment should be removed.

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

 This is not a requirement of gen-rdatacode.  It's a consequence of the
 fact that the base class Rdata declares the copy constructor and
 operator= as private methods (to make it non copyable, which is for
 avoiding unexpected slicing).  Even if they are private in terms of
 accessibility, the compiler finds it when it tries to resolve the
 definition in a derived class and then throw a compilation error due
 to the accessibility restriction.  So, admittedly it's a bit
 inconvenient, we always have to define these methods in derived
 classes explicitly.

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