Re: [lmi] virtual inheritance from noncopyable/obstruct_slicing

[Greg Chicares]

On 2008-06-04 19:54Z, Vadim Zeitlin wrote:
> On Wed, 04 Jun 2008 14:48:07 +0000 Greg Chicares <address@hidden> wrote:
[...]
> 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:

You're in good company--that's what Dietmar said:
  http://lists.boost.org/Archives/boost/2001/09/17372.php
However, I'm firmly in the other camp, with Daniel Frey:
  http://lists.boost.org/Archives/boost/2001/09/17374.php
so I'm in good company as well.

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

We're on opposite sides of that debate, but I don't think either
of us has to switch sides, because Cacciola seems to have a
solution both sides can be comfortable with.

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

Okay, let's do that.

> 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

Yes, please.

> (unless you agree with the even simpler solution of
> deriving from boost::noncopyable only in the base classes).

No, I don't think either of us is going to change sides on that
issue.

> [switching to MSVC-specific part now]
[...]
> 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.

Okay, you can't hope to impose a nondefault choice on IDE users.

And the same kind of problem could arise with a different
compiler. I'm thinking of borland's '-Vmv'.

Maybe it'd be different if the standard guaranteed that wx can
cast member-function pointers the way it does--then you could say
these compilers are broken--but I suspect that's not the case.

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

Exotic #pragmata are often broken in horrible ways; they don't
get tested well, and they can change subtly from one compiler
version to the next. That's my experience with borland. So thanks
for mentioning it, but I'm no more optimistic than you about it.

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

Well, if practical problems with msvc mean that we must in effect
remove virtual inheritance from the language, then I must object.
But I think there's a reasonable solution here, too....

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

I agree with you and Cacciola that ':virtual boost::noncopyable'
is a kludge that we should get rid of.

But the problem that 'obstruct_slicing' solves cannot be solved
AFAIK without virtual inheritance. However, consider:

class CensusView
    :public ViewEx
    ,private noncopyable_lmi_t<ViewEx> // Cacciola's idea
#if defined __SOME_MSVC_MACRO
    ,virtual private obstruct_slicing<CensusView>
#endif // defined __SOME_MSVC_MACRO
{...

I'd say it's enough to document the reason for '#if' in
'obstruct_slicing.hpp', once and only once. But I wouldn't
argue with adding a comment everywhere, pointing to that:

#if defined __SOME_MSVC_MACRO
    // Avoid msvc problem--see 'obstruct_slicing' header.
    ,virtual private obstruct_slicing<CensusView>
#endif // defined __SOME_MSVC_MACRO

Whatever problem I seek to solve via 'obstruct_slicing',
remains solved, because I use gcc. Whatever msvc problem
you seek to avoid, is avoided.

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

I think we should follow Cacciola's suggestion, and then we don't
have to look for odd workarounds in special cases. Maybe we can
even replace the existing workaround in 'any_member.hpp'.