sigc::trackable::~trackable() is not virtual

sigc::trackable::~trackable() is not virtual. 

this can lead to problems when using virtual inheritance of
a
sigc::trackable, at least with gcc 3.4-4.1. consider this
tree:

class A : public virtual sigc::trackable {}
class B : public virtual sigc::trackable {}
class C : public A, public B {}

this leads to a compiler-induced disaster when destroying a
C. or at
least it can (i just spent most of a week tracking this
down).

why is this destructor not virtual by default? i thought it
was widely
accepted good practice that if the class is ever intended to
be
inheritable, its destructor should be virtual? 

--p


	

_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thu, 2006-10-26 at 08:58 -0400, Paul Davis wrote:
> why is this destructor not virtual by default? i
thought it was widely
> accepted good practice that if the class is ever
intended to be
> inheritable, its destructor should be virtual?  

Not necessarily. The destructor should be virual if it is
intended to be
used polymorphically.

But yes, this would be useful to me sometimes too. I believe
the choice
was made once for performance.

And you might actually be better off with your virtual
inheritance
elsewhere. Obviously it works for gtkmm, which
multiply-inherits
sigc::trackable. Virtual inheritance is a very odd thing and
doesn't act
exactly as you might expect.

-- 
Murray Cumming
murraycmurrayc.com
www.murrayc.com
www.openismus.com

_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thu, 2006-10-26 at 15:29 +0200, Murray Cumming wrote:
> On Thu, 2006-10-26 at 08:58 -0400, Paul Davis wrote:
> > why is this destructor not virtual by default? i
thought it was widely
> > accepted good practice that if the class is ever
intended to be
> > inheritable, its destructor should be virtual?  
> 
> Not necessarily. The destructor should be virual if it
is intended to be
> used polymorphically.
> 
> But yes, this would be useful to me sometimes too. I
believe the choice
> was made once for performance.

now this is silly. do you realize that every time you do
this:

	someSignal.connect (mem_fun (someObject,
&SomeObject::method))

that the temporary slot (the one passed to the connect()
method) creates
its own trackable, adds a destroy notification callback to
the
trackable's callback list, then removes that callback and
then destroys
its? this is for every single connect, every time. the cost
of this
extra work (including heap allocation of an std::list)
versus a virtual
function call when destroying a trackable makes this concern
seem
misplaced to me. avoiding the temporary slot from doing all
this stuff
would be much more productive in terms of cycles not wasted.

> And you might actually be better off with your virtual
inheritance
> elsewhere. Obviously it works for gtkmm, which
multiply-inherits
> sigc::trackable. Virtual inheritance is a very odd
thing and doesn't act
> exactly as you might expect.

no kidding. 5 days of valgrinding and gdb-ing and reading
MB's of
debugging output finally convinced me that VI was the issue.
i used VI
to avoid having to constantly specifiy which trackable of
the multiple
trackables would otherwise be present i meant, which was
apparently what
VI was intended for. instead, it led to gcc completely
screwing up the
class layout and consistently messing up object destruction.
i've heard
of at least one other story that matches this experience
with VI.

--p



_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thu, 2006-10-26 at 09:41 -0400, Paul Davis wrote:
> the cost of this
> extra work (including heap allocation of an std::list)
versus a
> virtual
> function call when destroying a trackable makes this
concern seem
> misplaced to me. avoiding the temporary slot from doing
all this stuff
> would be much more productive in terms of cycles not
wasted.

(if that's correct. I have no idea. I haven't intestigated)

That's a once-per-connection cost versus an increase in size
for all
objects that derive from it.

The decision was made a long time ago, not by me, and it
can't be
undecided now.

> > And you might actually be better off with your
virtual inheritance
> > elsewhere. Obviously it works for gtkmm, which
multiply-inherits
> > sigc::trackable. Virtual inheritance is a very odd
thing and doesn't
> act
> > exactly as you might expect.
> 
> no kidding. 5 days of valgrinding and gdb-ing and
reading MB's of
> debugging output finally convinced me that VI was the
issue. i used VI
> to avoid having to constantly specifiy which trackable
of the multiple
> trackables would otherwise be present i meant, which
was apparently
> what
> VI was intended for. instead, it led to gcc completely
screwing up the
> class layout and consistently messing up object
destruction. i've
> heard
> of at least one other story that matches this
experience with VI. 

I'm not aware of any remaining gcc bugs with virtual
inheritance, though
we had some a few years ago. I was suggesting that the
concepts and
syntax are difficult, and you might have your virtual
inheritance in the
wrong place. It's worth investigating what gtkmm does.

Bakery also does some complex virtual inheritance.
-- 
Murray Cumming
murraycmurrayc.com
www.murrayc.com
www.openismus.com

_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thursday 26 October 2006 15:41, Paul Davis wrote:
> > But yes, this would be useful to me sometimes too.
I believe the
> > choice was made once for performance.
>
> now this is silly. do you realize that every time you
do this:
>
> 	someSignal.connect (mem_fun (someObject,
&SomeObject::method))
>
> that the temporary slot (the one passed to the
connect() method)
> creates its own trackable, adds a destroy notification
callback to
...
> concern seem misplaced to me. avoiding the temporary
slot
> from doing all this stuff would be much more productive
in
> terms of cycles not wasted. 

It's not silly at all!!!! You're relying on UNDEFINED
BEHAVIOUR when you 
subclass a type which doesn't have a virtual dtor.

Consider this: 

struct A : public sigc::trackable { ... };

sigc::trackable * foo = new A;
delete foo;

You've now officially got UNDEFINED BEHAVIOUR, according to
the C++ 
standard. It is illegal to delete a pointer to a derived
type through a 
parent-type pointer if the parent type's dtor is not
virtual.

Virtual inhertiance has nothing to do with the dtor, AFAIK,
but with the 
way that the inheritance itself is declared:

struct A: public virtual B { ... };

For lots of info:

http://www.parashift.com/c++-faq-lite/multiple-in
heritance.html

http://www.parashift.com/c++-faq-lite/virtu
al-functions.html#faq-20.7



-- 
----- stephans11n.net   http://s11n.net
"...pleasure is a grace and is not obedient to the
commands
of the will." -- Alan W. Watts
_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thu, 2006-10-26 at 17:19 +0200, stephan beal wrote:
> It's not silly at all!!!! You're relying on UNDEFINED
BEHAVIOUR when
> you 
> subclass a type which doesn't have a virtual dtor. 

That's not true. It's undefined (or quite clearly, a memory
leak, I
think), if you use it polymorphically (because you are
likely to delete
it via the base class. That's why compilers warn if you have
virtual
methods without a virtual destructor.

In this case, nobody is very likely to use the
sigc::trackable base
class directly.

There are perfectly valid reasons to have base classes
without virtual
destructors. Performance is one possible reason.

-- 
Murray Cumming
murraycmurrayc.com
www.murrayc.com
www.openismus.com

_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thu, 2006-10-26 at 17:19 +0200, stephan beal wrote:

> It's not silly at all!!!! You're relying on UNDEFINED
BEHAVIOUR when you 

just to be clear, what i said about "this is
silly" was focusing on the
size of trackable and derivatives when runtime performance
was so poor.

> subclass a type which doesn't have a virtual dtor.
> 
> Consider this: 
> 
> struct A : public sigc::trackable { ... };
> 
> sigc::trackable * foo = new A;
> delete foo;

see this note in trackable.h:

  /*virtual ~trackable() {} */  /* we would need a virtual
dtor for
users
                                   who insist on using
"trackable*" as
                                   pointer type for their
own derived
objects */

and smile 

--p


_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thu, 2006-10-26 at 12:06 -0400, Paul Davis wrote:
> just to be clear, what i said about "this is
silly" was focusing on
> the
> size of trackable and derivatives when runtime
performance was so
> poor 

Karl Nelson was quite obsessed with runtime performance
(though probably
more on signal emission than signal connection,
understandably), and
runtime memory use. If you can improve it, for some future
parallel-installable version of libsigc++, and prove that
it's an
improvement, you might try to provide a patch.

-- 
Murray Cumming
murraycmurrayc.com
www.murrayc.com
www.openismus.com

_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thursday 26 October 2006 19:34, Murray Cumming wrote:
> On Thu, 2006-10-26 at 12:06 -0400, Paul Davis wrote:
> > just to be clear, what i said about "this is
silly" was focusing on
> > the
> > size of trackable and derivatives when runtime
performance was so
> > poor
>
> Karl Nelson was quite obsessed with runtime performance
(though
> probably more on signal emission than signal
connection,
> understandably), and runtime memory use. If you can
improve it, for
> some future
> parallel-installable version of libsigc++, and prove
that it's an
> improvement, you might try to provide a patch.

IMO, any change which fixes undefined behaviour should be
made 
regardless of any perceived performance hit.

As you said earlier:

> It's undefined (or quite clearly, a memory leak, I
> think), if you use it polymorphically (because you are
likely to
> delete it via the base class. 

Whether it's undefined only when i delete using a parent
pointer or a 
child pointer is irrelevant. Opening the door to undefined
behaviour 
leads to undefined behaviour. The door shouldn't even be
open.

> That's why compilers warn if you have 
> virtual methods without a virtual destructor.

Not all do. Recent gcc's do, but several common compilers do
not.

> In this case, nobody is very likely to use the
sigc::trackable base
> class directly.

i'm not so optimistic about that point. The moment someone
does use a 
trackable like that, they've got undefined behaviour and
probably won't 
know why. They know that trackable is *meant* to be
subclassed, and 
therefor presumably has the required virtual dtor. It's just
good, safe 
practice to do so.

To quote Scott Meyers, More Effective C++ 3rd Edition, Item
51, page 
255-256:

"Interestingly, the size_t value C++ passes to operator
delete may be 
incorrect if the object being deleted was derived from a
base class 
lacking a virtual destructor. This is reason enough for
making sure 
your base classes have virtual destructors, but Item 7
describes a 
second, arguably better reason."

In Item 7, page 44, he summarizes:

- Polymorphi base classes should declare virtual
destructors. If a class 
has any virtual functions, it should have a virtual
destructor.

- Classes not designed to be base classes or not designed to
be used 
polymorphically should not declare virtual destructors.

We could nit-pick on whether trackable meets that last
point, but IMO 
*any* public subclassing is inherently polymorphism because
it allows 
an implicit conversion from a derived to a parent pointer
type:

struct foo : public trackable { ... };

trackable * t = new foo;

This is already polymorphism, regardless of any function
calls. A dtor 
is a function call, too, so i will argue that any public
subclassing is 
automatic polymorphic behaviour.


-- 
----- stephans11n.net   http://s11n.net
"...pleasure is a grace and is not obedient to the
commands
of the will." -- Alan W. Watts
_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list

On Thu, 2006-10-26 at 20:36 +0200, stephan beal wrote:
> IMO, any change which fixes undefined behaviour should
be made 
> regardless of any perceived performance hit.

If you can submit a test case I will consider it. I don't
think there is
any bug, so you have to show that it exists.

> Whether it's undefined only when i delete using a
parent pointer or a 
> child pointer is irrelevant. Opening the door to
undefined behaviour 
> leads to undefined behaviour. The door shouldn't even
be open.

C++ does not protect you from errors. You must know what you
are doing.

Likewise, you can provide an incorrect reference to a
std::list<>, and
it will cause a crash (or "undefined behaviour"). 

[snip]
> i'm not so optimistic about that point. The moment
someone does use a 
> trackable like that, they've got undefined behaviour
and probably won't 
> know why.

They must know why. This is very basic C++.

"Meant to be subclassed" does not mean "meant
to be used
polymorphically". I've made that point repeatedly now.

[snip]
> - Classes not designed to be base classes or not
designed to be used 
> polymorphically should not declare virtual destructors.
[snip]

> struct foo : public trackable { ... };
> 
> trackable * t = new foo;
> 
> This is already polymorphism, regardless of any
function calls.

So, don't do that.

>  A dtor 
> is a function call, too, so i will argue that any
public subclassing is 
> automatic polymorphic behaviour.

That's quite clearly wrong. C++ is not as simple as you'd
like it to be.

-- 
Murray Cumming
murraycmurrayc.com
www.murrayc.com
www.openismus.com

_______________________________________________
libsigc-list mailing list
libsigc-listgnome.org
h
ttp://mail.gnome.org/mailman/listinfo/libsigc-list