Re[2]: [lmi] virtual inheritance from noncopyable/obstruct_slicing

[Vadim Zeitlin]

On Wed, 04 Jun 2008 14:48:07 +0000 Greg Chicares <address@hidden> wrote:

GC> I try to consider "special member functions" carefully when I'm
GC> designing a class:

 This is definitely an excellent advice to any C++ programmer and far from
me to disagree with anything you wrote -- after all, I taught exactly this
to many people myself. It goes without saying that deriving from
boost::noncopyable (or an equivalent) should still be done, I only question
the need to do it virtually.

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

 I see the reason for deriving virtually now but, now that I see it :-), I
can argue against it: IMO it's wrong to use the virtual inheritance
mechanism which has a clean logical meaning in C++ to work around a
compiler warning. Personally I think that deriving from a base class which
is non-copyable is enough, i.e. I disagree with the objection expressed in
the first of the links above: if class B is changed to not derive from
class A any more, you would definitely need to review its copying semantics
anyhow because they may change after a refactoring serious enough to change
the classes hierarchy. So IMO it's enough to simply derive from
boost::noncopyable in the base class and not derive from anything at all in
the derived one(s) -- they're already non-copyable anyhow.

 But if you think it's a serious concern then I'd definitely prefer to use
Cacciola's solution. And I absolutely agree with his criticism in the last
link and generally believe that something as heavy as virtual inheritance
is inappropriate for solving such small, local, tactical problem.

GC> http://lists.boost.org/Archives/boost/2001/09/17385.php
GC> ...
GC> | struct A : noncopyable {} ;
GC> | struct B : A, noncopyable_t<B> {} ;
GC> | struct C : B, noncopyable_t<C> {} ;
GC> 
GC> would you regard this:
GC> 
GC>   struct A : noncopyable_t<A> {} ;
GC>   struct B : A, noncopyable_t<B> {} ;
GC>   struct C : B, noncopyable_t<C> {} ;
GC> 
GC> (which is a consequence of what I think you're saying) as an
GC> improvement (provided that it's correct--I haven't tested it)?

 This would definitely be an improvement, yes.

GC> Or would you say that
GC>   struct X : noncopyable {} ;
GC> is better where X isn't used as a base class, because it's
GC> easier and avoids harmful cut-and-pastos like
GC>   struct XYZ : noncopyable_t<ABC> {} ;
GC> ?

 I think this is the best because it's the simplest and IMO is good enough.

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

 Ok, I can do this (unless you agree with the even simpler solution of
deriving from boost::noncopyable only in the base classes).


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

 I think it's rather important but I don't have any hard numbers to prove
it. There is however one overarching consideration which completely trumps
any efficiency concerns: if we built wxWidgets with /vmg, then any program
created using the "New Project" function of the MSVC IDE (which is how 99%
of people use it) or compiled with cl.exe with default options would
silently and mysteriously crash during run-time. So using /vmg by default
is really not an option.

 To be fair, there is also a possibility of solving this problem otherwise,
by using Microsoft-specific "#pragma pointers_to_members(full_generality)"
in wx code. I didn't test it however and I'm still not sure about whether
this really solves the problem (it looks like it should though) and while
this would avoid the problem as long as people only use wx, it would still
create potential interoperability problems with the with other libraries
compiled with the default options. I could explore this option further but
I'm not too optimistic about it, it seems to me that nothing good can come
from using non-default pointer representation, especially considering the
severity of the problems it causes in case of mismatch.


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

 No, any classes using virtual base class. All those derived from ViewEx do
qualify because of virtual inheritance from boost::noncopyable. But this
can be avoided using either of the techniques described above. What I'm
worried about are classes deriving from obstruct_slicing (in which case
inheritance must be virtual for it to work) and handling wx events. There
are probably not that many of them (should I make a full list?) but I just
don't see what can be done about them if obstruct_slicing has to be kept
even for the already non-copyable classes.

GC> Perhaps, for a limited number of classes, we can just use
GC> >   class T : private boost::noncopyable
GC> as you suggest--or, if that wouldn't compile because a base
GC> class already derives from boost::noncopyable, we could think
GC> about this alternative from 'any_member.hpp':

 FWIW wx has DECLARE_NO_COPY_CLASS() which does this (and as this problem
only arise for the classes used as wx event handlers we presumably could
use an existing wx macro... or, of course, we could define an equivalent
LMI_NO_COPY_CLASS one).

 Thanks,
VZ