Re: [lmi] virtual inheritance from noncopyable/obstruct_slicing

[Greg Chicares]

On 2008-06-01 21:06Z, Vadim Zeitlin wrote:
> 
>  This is another email with questions which originally arose from the MSVC
> build problems but seem to merit attention even if MSVC is not a concern.

All right. Sometimes another toolset exposes real problems.

>  The first question is: why so many LMI classes inherit from both
> boost::noncopyable and obstruct_slicing? If I understand the purpose of the
> latter class correctly, it is used to prevent code like this from
> compiling:
> 
>       class B : obstruct_slicing<B> { };
>       class D : public B { };
> 
>       D d;
>       B b;
>       b = d;

I'll change its documentation, which was misleading:

 /// Is it desirable to "prevent derivation" from a class? Some say
 /// the lack of a virtual destructor is a warning that any competent
 /// C++ programmer would consider thoughtfully; others, that feasible
-/// safeguards ought to be used where slicing seems to be a danger.
+/// safeguards ought to be used where there might be any danger.
 ///
-/// The purpose of such a safeguard is only to prevent slicing. It may
+/// One purpose of such a safeguard is to prevent slicing. It may also
 /// be useful, for instance, for proving that it's safe to revise a
 /// legacy class that appears not to need a virtual dtor, but has one
 /// anyway, perhaps because of an obsolete "make all dtors virtual"
 /// guideline. After such a proof, it could be removed if wanted, but

I think that change makes it agree better with this:

http://www.research.att.com/~bs/bs_faq2.html#no-derivation

which suggests two valid purposes:

| to be sure that I can copy objects without fear of slicing

| I have seen examples where performance-critical functions had
| been made virtual for no good reason, just because "that's the
| way we usually do it".

of which I had overemphasized the first.

> But if B already derives from boost::noncopyable this seems to be
> impossible anyhow because B (and hence D, unless someone specifically took
> care to implement D assignment operator and/or copy ctor bypassing those of
> the base class) doesn't have an accessible assignment operator. So what is
> the reason for deriving from both classes, wouldn't just boost::noncopyable
> be enough?

Perhaps the combination of both is overkill, but I see no harm in
multiple redundant safety features.

I try to consider "special member functions" carefully when I'm
designing a class:
 - Should the dtor be virtual?
 - Do I need to implement a copy ctor and operator=()?
Bad things can happen automatically if I fail to think those
things through well--unless I derive from these utility classes.
For example, consider:

  // Hesitate to derive from this class because it doesn't have a
  // virtual dtor, or to copy it because the implicitly-defined
  // copy ctor and operator=() might do the wrong thing. All
  // programmers are responsible for reading and following these
  // guidelines carefully! If you don't do so, it's your fault!
  // You have been warned! Don't complain to me!
  class actuarial_table {...};

Yeah, right--like anyone's going to read that. Not.

It's safer if I write this by default:

  class actuarial_table
      :private boost::noncopyable
      ,virtual private obstruct_slicing<actuarial_table>
  {...};

Now the compiler emits an error message if that's used in such
unanticipated ways. I don't have to think about special member
functions unless they turn out to be needed; often they don't.
That's one safety-enhanced exemplar that can be cut and pasted;
here's another:

  /// Implicitly-declared special member functions do the right thing.
  class foo
  {...no pointer members...};

If I knew a way to enforce "no pointer members" via derivation
from a utility class, then I'd do so instead of writing that
comment.

>  The second question is why some classes (albeit not all) derive from
> boost::noncopyable virtually. This seems to be totally unnecessary and
> other than some dubious consideration of symmetry

The reason isn't symmetry.

> with obstruct_slicing I
> just don't see any reason for this.

I documented it only once, in 'view_ex.hpp', as follows; but it
looks like the idiom is used only in this class and in classes
derived from it.

// INELEGANT !! If both a base and a derived class derive nonvirtually
// from boost::noncopyable
//    ,private boost::noncopyable
// then gcc will complain
//   warning: direct base `boost::noncopyable' inaccessible in
//   [derived classes] due to ambiguity
// Cacciola's noncopyable<T> solution here
//   http://lists.boost.org/MailArchives/boost/msg17485.php
//   http://lists.boost.org/MailArchives/boost/msg17012.php
// is best; no good reason for dismissing it was given on the boost
// mailing list, but presumably they didn't want to require typing
// the template argument. The workaround used here is rightfully
// criticized by Cacciola:
//   http://lists.boost.org/MailArchives/boost/msg17491.php
// That criticism matters little in this particular case, but perhaps
// it would be better to use Cacciola's idea instead of boost's.

class ViewEx
    :public wxView
    ,virtual private boost::noncopyable

> And even if I'm missing something again
> there is still the issue of deriving from it virtually in some classes but
> not the others while I think the same kind of inheritance should be used
> everywhere, i.e. in any case something needs to be done.

This is a symmetry argument that I could sympathize with. Thus,
where Cacciola writes:

http://lists.boost.org/Archives/boost/2001/09/17385.php
...
| struct A : noncopyable {} ;
| struct B : A, noncopyable_t<B> {} ;
| struct C : B, noncopyable_t<C> {} ;

would you regard this:

  struct A : noncopyable_t<A> {} ;
  struct B : A, noncopyable_t<B> {} ;
  struct C : B, noncopyable_t<C> {} ;

(which is a consequence of what I think you're saying) as an
improvement (provided that it's correct--I haven't tested it)?
Or would you say that
  struct X : noncopyable {} ;
is better where X isn't used as a base class, because it's
easier and avoids harmful cut-and-pastos like
  struct XYZ : noncopyable_t<ABC> {} ;
?

I'd welcome a patch to add, e.g., 'noncopyable_lmi.hpp'
(using Cacciola's technique, and repeating the citations above
to give him proper credit), and to use it wherever appropriate
in lieu of boost::noncopyable.

> And I believe that
> the virtual keyword should be removed from the classes that use it, what do
> you think?

I haven't tried it, but the 'view_ex.hpp' documentation copied
above seems to suggest that gcc would complain; therefore...

>  To summarize both of these questions, I'd like to replace all occurrences
> of
> 
>       class T : [virtual] private boost::noncopyable,
>                 virtual private obstruct_slicing<T>
> 
> with just
> 
>       class T : private boost::noncopyable

...I suspect that gcc would complain here, too. But I have
another reason to hesitate: I'm not sure these safety features
are useless here today and would never be useful. To answer that
question would require thought, where the original point was to
gain safety without requiring any thought. Still, we could think
about it if there's a good enough reason....

> My interest in doing this, to return to the MSVC angle, is that due to a
> compiler limitation (which is not quite a bug as it's probably meant to be
> an optimization and it's arguably indeed a rather successful one) the
> classes using virtual inheritance can't be used as wx event handlers with
> MSVC by default: the representation of the pointer to members of these
> classes is incompatible with the one used by wxEvtHandler and so any
> attempt to use a method of such class as an event handler (either using an
> event table entry or with wxEvtHandler::Connect()) results in a very
> unclear (and this is definitely a bug) compiler error message
> 
>       error C2102: '&' requires l-value
> 
> This limitation can be fixed by using a special compiler option (/vmg)
> which instructs it to use the most generic representation of pointer to
> members. But as this option is non-default and has a cost (it doubles the
> size of all pointer to members, whether they use virtual inheritance or
> not), it is not used by wxWidgets MSVC build by default and hence I still
> have to maintain a separate wx build (or rather (N+1)-st separate wx build)
> just for LMI which was something I hoped to avoid. And the situation is
> especially pernicious because the code compiled with and without /vmg can
> be linked together without any errors but dies horrible during run-time (as
> you would expect because different object modules use differently sized
> pointers).

How important is this optimization, though? I'm willing to be
persuaded, but I wonder: why don't you use the "/vmg" option
everywhere? I understand why pointers-to-member may have
different sizes, but what I don't see is why that matters in
an event handler. I'm thinking of WM_COMMAND handlers, though,
where an extra millisecond isn't noticeable, and maybe your
concern is something like mouse events that have proven to
make an application feel sluggish based on actual observation?

>  So it would be really great if virtual inheritance could be avoided in the
> classes used as wx event handlers. Of course, I wasn't about to ask you to
> consider not using virtual inheritance just because of this MSVC-only
> problem, but if it turns out that virtual inheritance is not needed anyhow,
> I'd be very glad to get rid of it. Would this be possible?

Can you say which classes? Is it only those derived from ViewEx?
Perhaps, for a limited number of classes, we can just use
>       class T : private boost::noncopyable
as you suggest--or, if that wouldn't compile because a base
class already derives from boost::noncopyable, we could think
about this alternative from 'any_member.hpp':

// By its nature, this class is Noncopyable: it holds a map of
// pointers to member, which need to be initialized instead of copied
// when a derived class is copied. Its Noncopyability is implemented
// natively: deriving from boost::noncopyable would prevent class C
// from deriving from MemberSymbolTable<C> and boost::noncopyable.
...
template<typename ClassType>
class MemberSymbolTable
{
...
  private:
    MemberSymbolTable(MemberSymbolTable const&);
    MemberSymbolTable& operator=(MemberSymbolTable const&);