Re: [Texmacs-dev] [PATCH] solves GCC 3.2 auto_save segfault for me...

[Igor V. Kovalenko]

Joris van der Hoeven wrote:
> > > > struct concrete_struct {
> > > >   int ref_count;
> > > >   inline concrete_struct (): ref_count (1) { DEBUG(concrete_count++); }
> > > > -  inline ~concrete_struct () { DEBUG(concrete_count--); }
> > > > +  inline virtual ~concrete_struct () { DEBUG(concrete_count--); }
> > > > };
> > >
> > >
> > > The classes concrete_struct and abstract_struct are really base classes
> > > and *not* abstract classes: we never explicitly allocate a concrete_struct
> > > or an abstract_struct. Moreover, the destructor does nothing when DEBUG
> > > is inactive. We might just add "DEBUG(virtual)" to be extremely correct.
> >
> > In my opinion this "virtualization" has an effect to make all destructors
> > in derived classes virtualized, so pointer can be deleted without explicitly
> > accessing the data.
>
>
> Yes, I understand, but there will only be a difference when you
> destroy an abstract object (like a  "box_rep" or a "widget_rep"
> in TeXmacs, in which case we use virtual destructors). Once again,
> if a class is bound to be used as a base class only and *never*
> as an abstract class, then you should not use virtual destructors,
> and certainly not if they do nothing!

Agreed completely.

> I remind you that we never declare variables of type concrete_rep,
> so concrete_rep is only used as a pure base class.
> If the "virtual" directive is missing somewhere,
> then it is missing in one of the derived classes of concrete_struct.

Agreed completely. In case of removing destroy_tree_rep() call the change
should be in adding a virtual destructor to tree_rep.

> This might indeed be the case; in order to check this,
> one just has to look at all derived classes which are again
> base classes for other classes.

I use -Wnon-virtual-dtor, no gcc complains so far...

> > > > +inline tree::~tree ()
> > > > +{
> > > > +    if ((--rep->ref_count)==0) {
> > > > +        //destroy_tree_rep (rep);
> > > > +        delete rep;
> > > > +        rep = NULL;
> > > > +    }
> > > > +}
> > >
> > >
> > > Again, the function destroy_tree_rep explicitly does the "delete rep"
> > > depending on the kind of tree (string or compound). Indeed, this type
> > > can be determined as a function of "op", so destroy_tree_rep plays
> > > the rôle of a virtual destructor *without* the extra space overhead.
> > > It may still be interesting though to add the line "rep = NULL"
> > > to destroy_tree_rep.
> >
> > Nope, it does a wild cast do destruct an object. Correct?
>
>
> Well, a cast yes, but not a wild cast, since we *know* the cast to be correct.

Yep.

Still, yet another path being walked in bug detection...

--
Regards,
Igor V. Kovalenko    mailto: iko at crec dot mipt dot ru