Opened 8 years ago

Closed 6 years ago

#2144 closed enhancement (wontfix)

add nonassignable super class

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Remaining BIND10 tickets
Component: Unclassified Version: bind10-old
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

When a class is defined with a const member and no explicit assignment operator the compiler can't build the operator and some raise a warning (4512 with MSVC).
The idea is to clone the noncopyable super class from boost and to derived all impacted classes (and structures?) from it. BTW I suggest to do the same for noncopyable (where both the copy constructor and the operator = are declared but left undefined).
include files should be in util.
I'll provide the list of classes and structures.

Subtickets

Change History (20)

comment:1 Changed 8 years ago by shane

  • Milestone New Tasks deleted

comment:2 Changed 8 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to assigned

comment:3 Changed 8 years ago by fdupont

  • Status changed from assigned to accepted

comment:4 Changed 8 years ago by fdupont

Done in trac2144 (so please review and merge).
Comments:

  • includes in lib/util (seems the best current place)
  • lib/dns LabelSequence requires an explicit assignment operator
  • src/lib/datasrc/tests/database_unittest.cc ExpectedRRset is wrong (C vs C++ style)

comment:5 Changed 8 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

comment:6 follow-up: Changed 7 years ago by shane

  • Milestone set to Next-Sprint-Proposed

I haven't looked at the ticket, but our coding guidelines suggest we actually use the Boost class, rather than invent a new one:

http://bind10.isc.org/wiki/CodingGuidelines#Non-copyableClasses

Anyway, if this work is done we can go ahead and review and merge it.

comment:7 in reply to: ↑ 6 Changed 7 years ago by fdupont

Replying to shane:

I haven't looked at the ticket, but our coding guidelines suggest we actually use the Boost class, rather than invent a new one:

http://bind10.isc.org/wiki/CodingGuidelines#Non-copyableClasses

Anyway, if this work is done we can go ahead and review and merge it.

copyable != assignable

and BTW the coding guidelines apply, i.e., it is easier and cleaner to inherit from a not-xxxable class than to make related methods private by hand class by class.

comment:8 Changed 7 years ago by shane

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:9 Changed 7 years ago by jelte

  • Estimated Difficulty changed from 3 to 2
  • Milestone changed from Next-Sprint-Proposed to Sprint-20130402

As discussed in planning meeting, I am reducing the estimated time to 2

comment:10 follow-up: Changed 7 years ago by jinmei

general

I first created a new branch from the original trac2144 and rebased it
on the latest master, as there have been many changes since the
original branch. The new branch is trac2144-2. I suggest we made
further changes on this new branch (and we should probably just remove
the old one to avoid confusion).

I've not examined all cases where noncopyable or nonassignable is
newly introduced to confirm they are really necessary and whether
noncopyable/nonassignable is the best choice (especially because my
compiler doesn't warn about the things reported in this ticket). It's
also possible that there are some new cases where noncopyable or
nonassignable would now be needed.

In addition to conflict resolution, I made some other cleanups in my
own commits. 5b4d247 should be trivial, and I believe the rest is not
problematic, but you may want to check them.

nonassignable.h and noncopyable.h

  • First off: do we need our own noncopyable, if it's just a copy of Boost?
  • Please add documentation: brief description of what it is, the purpose of these classes, in which case we should use which, and why, etc.
  • Shouldn't we also copy Boost copyright?
    /// From boost/noncopyable.hpp
    
    even though the definition is quite trivial.
  • what's the intent of the postfix underscore in noncopyable_?
    namespace noncopyable_ {
    
    and the typedef?
    typedef noncopyable_::noncopyable noncopyable;
    

labelsequence

  • (Especially) if we define our own operator=, it should be tested
  • And, if we really need it, I guess we don't need to include this:
    #include <util/nonassignable.h>
    
  • 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.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to fdupont

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by fdupont

Replying to jinmei:

nonassignable.h and noncopyable.h

  • First off: do we need our own noncopyable, if it's just a copy of Boost?

=> a copy

  • 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:

The compiler will also generate an assignment operator function for a class that does not define one. This assignment operator is a memberwise copy of the data members of an object. Because const data items cannot be modified after initialization, if the class contains a const item, the default assignment operator would not work. Another cause of the C4512 warning is a declaration of a nonstatic data member of reference type.

  • Shouldn't we also copy Boost copyright?
    /// From boost/noncopyable.hpp
    
    even though the definition is quite trivial.

=> I don't know the exact policy but IMHO we should.

  • what's the intent of the postfix underscore in noncopyable_?
    namespace noncopyable_ {
    
    and the typedef?
    typedef noncopyable_::noncopyable noncopyable;
    

=> it is from Boost so ask a Boost person...

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

comment:13 Changed 7 years ago by jinmei

  • Owner changed from fdupont to jinmei

comment:14 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by jinmei

Replying to 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. My question is why not just keep using
boost::noncopyable.

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

  • Shouldn't we also copy Boost copyright?
    /// 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.

  • what's the intent of the postfix underscore in noncopyable_?
    namespace noncopyable_ {
    
    and the typedef?
    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 and updating the
unittest so it doesn't use the 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.

comment:15 Changed 7 years ago by jinmei

  • Owner changed from jinmei to fdupont

comment:16 follow-up: Changed 7 years ago by muks

  • Milestone Sprint-20130423 deleted

Removing from the current sprint.

comment:17 in reply to: ↑ 16 Changed 7 years ago by jinmei

Replying to muks:

Removing from the current sprint.

As this is not a blocking issue and it's now getting stuck in
progress, we've taken it back from the ongoing sprint. If and when
Francis has time to address review comments or someone else can take
over the developer side of the review process, we can resume the
ticket.

comment:18 in reply to: ↑ 14 Changed 7 years ago by fdupont

Replying to jinmei:

Replying to 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?
    /// 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_?
    namespace noncopyable_ {
    
    and the typedef?
    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.

comment:19 Changed 6 years ago by tomek

  • Milestone set to Remaining BIND10 tickets

comment:20 Changed 6 years ago by tomek

  • Resolution set to wontfix
  • Status changed from reviewing to closed
  • Version set to old-bind10

This issue is related to bind10 code that is no longer part of Kea.

If you are interested in BIND10/Bundy framework or its DNS components,
please check http://bundy-dns.de.

Closing ticket.

Note: See TracTickets for help on using tickets.