BIND 10 #2144: add nonassignable super class

[BIND 10 Development]

#2144: add nonassignable super class
-------------------------------------+-------------------------------------
            Reporter:  fdupont       |                        Owner:
                Type:  enhancement   |  fdupont
            Priority:  medium        |                       Status:
           Component:  Unclassified  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  2             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by fdupont):

 Replying to [comment:14 jinmei]:
 > Replying to [comment:12 fdupont]:
 >
 > > > '''nonassignable.h and noncopyable.h'''
 > > >
 > > > - First off: do we need our own noncopyable, if it's just a copy of
 > > >   Boost?
 > >
 > > => a copy
 >
 > I don't understand this.

 => oops, I believed the question was whether it was a copy.

 > My question is why not just keep using boost::noncopyable.

 => it is a matter of taste: copy and fork (and get an uniform
 nonassignable/noncopyable) or just fork (and get boost vs bind10 classes).
 >
 > > > - Please add documentation: brief description of what it is, the
 > > >   purpose of these classes, in which case we should use which, and
 > > >   why, etc.
 > >
 > > => do we need more than Boost doc? BTW as far as I can understand of
 Visual Studio explanations the use is for classes with const members:
 >
 > If we use the Boost version for noncopyable, obviously we don't need
 > to write anything ourselves for it (see the previous point).  And, for
 > nonassignable, there's no Boost implementation, much less
 > documentation, so we need to write it ourselves anyway.  And, how we
 > use them in our own code is probably specific to our own situation and
 > worth documenting.

 => IMHO we just need to explain:
  - what means nonassignable
  - when it should be used (const fields)
 For the first the boost doc can provide the material, for the second we
 have this ticket.

 > > > - Shouldn't we also copy Boost copyright?
 > > > {{{#!cpp
 > > > /// From boost/noncopyable.hpp
 > > > }}}
 > > >   even though the definition is quite trivial.
 > >
 > > => I don't know the exact policy but IMHO we should.
 >
 > I've added it to nonassignable.  For noncopyable, I still believe it's
 > better to just use the Boost version than maintaining our own copy.

 => as I explained it is only a matter of taste.

 > > > - what's the intent of the postfix underscore in `noncopyable_`?
 > > > {{{#!cpp
 > > > namespace noncopyable_ {
 > > > }}}
 > > >   and the typedef?
 > > > {{{#!cpp
 > > > typedef noncopyable_::noncopyable noncopyable;
 > > > }}}
 > >
 > > => it is from Boost so ask a Boost person...
 >
 > Actually the Boost implementation clarifies that:
 > {{{
 > namespace noncopyable_  // protection from unintended ADL
 > }}}
 > I'm not really sure if the class definition could cause any ADL
 > related troubles, but at least I can see some possibility.  I've added
 > the same single comment to nonassignable.h.  Again, for noncopyable, I
 > believe it's better to just use the Boost version than maintaining our
 > own copy so I didn't touch it.
 >
 > > > '''labelsequence'''
 > > >
 > > > - The restriction of the assignment operator seems quite counter
 > > >   intuitive and inconvenient.  What's the rationale of it?  At the
 > > >   very least this should be documented.
 > >
 > > => note I agree with Microsoft assignment operator and const members
 > > are not really compatible...
 >
 > We don't use operator= except in some part of labelsequence_unittest,
 > so I suggest making it assignable at least for now

 => I have no concern other than to assign a const is an illegal operation,
 even the const fields are assigned to the same values.

 > and updating the unittest so it doesn't use the operator.

 => I don't believe the unit test really requires the assignment
 operator.

 > Making `LabelSequence`
 > assignable may be useful, but it will increase the risk of
 > unintentional data sharing, so unless we really need it it's probably
 > safer to prohibit it explicitly.

 => fully agree.

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