Re: [PATCH] Make breakpoint subclasses inherit from breakpoint, add virtual destructor

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-05-03 11:08, Pedro Alves wrote:
> On 05/03/2017 03:36 PM, Simon Marchi wrote:
> > > > I want to replace the vectors in the various breakpoint subclasses 
> > > > by
> > > > std::vector.  The problem right now is that while breakpoint
> > > > subclasses are constructed using new, they are not properly deleted.
>
> > > I think "properly deleted" might not be 100% accurate.
> >
> > Hmm what do you suggest?  I could say:
> >
> >   ... their C++ destructor is not being called.
>
> Yeah.  It's not very important.  I was more referring to the fact that
> there's actual destruction of the "subclasses" than talking about
> properly-deleted-as-in-the-corresponding-dtor-is-called.
>
> To be crystal clear, I'd put "subclasses" in quotes, and add an 
> example:
>
> ~~~
>  I want to replace the vectors in the various breakpoint subclasses by
>  std::vector.  The problem right now is that while breakpoint
>  "subclasses" are constructed using new, they are not properly deleted:
>
>   struct syscall_catchpoint
>   {
>     /* The base class, old C style.  */
>     struct breakpoint base;
>
>     // trivial fields here
>   };
>
>   // first member is pointer-interconvertible.
>   breakpoint *bp = (breakpoint *) new syscall_catchpoint ();
>
>   // this calls ~breakpoint(), not ~syscall_catchpoint()...
>   delete bp;  // in delete_breakpoint
>
>  So if we add any non-trivially destructible field to
>  syscall_catchpoint, it won't be properly destructed...

Ok, this should make it clear.

> ~~~
>
> > You're right, it would be confusing and ugly to leave it with a
> > half-baked-dual-hybrid system with C++ destructors and dtor ops.  I'll
> > remove the dtor op, it shouldn't be much work, as you said.
>
> Thanks much!

FYI, I just looked at it, and it looks like the 
momentary_breakpoint/longjmp_breakpoint hierarchy will cause a bit of 
trouble.  longjmp_breakpoint has a dtor, but no struct/class of its own, 
so nowhere to put the destructor.  I think that to do it correctly, I'll 
have to introduce structs/classes for them and have:

  breakpoint
      ^
      |
  momentary_breakpoint
      ^
      |
  longjmp_breakpoint

To keep it clean, it might be better if I introduced the structs/classes 
for momentary_breakpoint and longjmp_breakpoint first with the old-style 
inheritance, and then converted them to "real" inheritance along with 
the other types.

I'll try that tonight, but if you have ideas in the mean time, I'm all 
ears.

Simon