[llvm] r238197 - Fix warning introduced in r238190 about lack of virtual destructor in MCObjectFileInfo.

[David Blaikie]

On Fri, May 29, 2015 at 6:37 AM, Daniel Sanders <Daniel.Sanders at imgtec.com>
wrote:

> > From: David Blaikie [mailto:dblaikie at gmail.com]
> > Sent: 29 May 2015 01:57
> > To: Daniel Sanders
> > Cc: llvm-commits at cs.uiuc.edu
> > Subject: Re: [llvm] r238197 - Fix warning introduced in r238190 about
> lack of virtual destructor in MCObjectFileInfo.
> >
> >
> >
> > On Thu, May 28, 2015 at 12:44 PM, Daniel Sanders <
> Daniel.Sanders at imgtec.com> wrote:
> > > > > Are these objects actually destroyed polymorphically? If not, we
> should make the dtor non-virtual
> > > > > and protected and the derived classes should be final.
> > > >
> > > > Sorry for the delay in replying to this. I had to revert r238197 and
> r238190 and I'm under time pressure
> > > > to fix and re-commit.
> > >
> > > Yes, they're destroyed via a TargetLoweringObjectFile pointer.
> > >
> > > Were we not doing that before? Or did we not have derived classes
> before? Curious/confused.
> >
> > MCObjectFileInfo didn't have a virtual destructor but didn't have
> virtual functions either. It's immediate
> > subclass (TargetLoweringObjectFile) had a virtual destructor and virtual
> functions. My patch added a virtual
> > function to MCObjectFileInfo which introduced the warning about having
> virtual functions without a virtual
> > destructor.
> >
> > But if it didn't change ownership (were things owned by
> TargetLoweringObjectFile* and are now
> > owned/destroyed by MCObjectFileInfo*?) then it shouldn't've needed a
> virtual dtor. Perhaps this base class's
> > dtor should be protected, then TargetLoweringObjectFile's dtor should be
> virtual (as it already must've been,
> > if that's the point of ownership?)?
>
> I think I see how to choose between the two now. If the virtual function
> is still there when I try to re-commit then I'll use a protected
> non-virtual destructor.
>

SGTM


>
> > If there's only one subclass, why does it need to be virtual in this
> base class?
>
> To be honest, the main reason for adding the virtual destructor was that
> the compiler was complaining about it not being virtual.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150529/321647d5/attachment.html>