[llvm] r254592 - Fix class SCEVPredicate has virtual functions and accessible non-virtual destructor.

[David Blaikie via llvm-commits]

On Thu, Dec 3, 2015 at 9:00 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> On Thursday, December 03, 2015 5:29 PM, David Blaikie wrote:
> On Thu, Dec 3, 2015 at 12:20 AM, Andy Gibbs wrote:
>
>> Author: andyg
>>> Date: Thu Dec  3 02:20:20 2015
>>> New Revision: 254592
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=254592&view=rev
>>> Log:
>>> Fix class SCEVPredicate has virtual functions and accessible non-virtual
>>> destructor.
>>>
>>>
>> Wait - why did the dtor need to be made virtual at all? These objects are
>> never
>> destroyed polymorphically, so far as I can tell.
>>
>> Could we just disable the GCC warning that was firing here instead? (if I
>> understand correctly and this was a GCC warning)
>>
>
> Fine, but IMHO the warning is a valid one.  The code may not currently
> destroy these
> objects polymorphically, but if that changes in future (for whatever
> reason) then
> its a potential memory leak.  Its one of those "golden rule" moments,
> isn't it? :o)
>

That's why the dtor is protected (& the derived classes with public
non-virtual dtors are final). This pattern is used quite a bit in LLVM &
the compiler diagnostics support it.

Both Clang and GCC don't warn in that case - but GCC does warn if that
class then has any friends.

I don't see that having a friend or two should change the tradeoff here -
you've exposed your internal details to those trusted pieces of code that,
much liket he class itself, are restricted to not doing the bad thing.

Not quite sure what golden rule(s) you're referring to.


>
> Rather than disable the warning, which could open up potential problems
> elsewhere
> not even related to this class,


Except that it's highly restricted because the dtor is protected and the
derived classes are final.


> I think it is better to fix the issue, even if it is
> technically/arguably a non-issue in this case.
>
> Its not like there is a huge overhead (code size, execution time, ...) to
> the fix.
> The classes already have vtables due to the other virtual methods, for
> example.
>

But we shouldn't be adding features just because of a compiler warning - we
should add features if/when we need them.

- Dave
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151203/3bdd0cbd/attachment.html>