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

[Andy Gibbs via llvm-commits]

On Thursday, December 03, 2015 6:06 PM, David Blaikie wrote:
  On Thu, Dec 3, 2015 at 9:00 AM, Andy Gibbs 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.

Please don't get me wrong here, I accept all your technical arguments even if I lean to a different stand-point, and if the decision is made to roll back this change, that's ok.

However, GCC *does* warn here.  My personal feeling is that I would rather see the GCC warnings in this case and not disable them simply because if they are disabled, then they are disabled throughout the project and somewhere where it would correctly pick up a real issue may be lost.  That said, I compile with -Werror mostly, so for me its not great to see the warnings either :o)

So, I come back to my initial stance which is it isn't really "adding features" and there is (unless you can correct me here) no tangible overhead, just additional safety.  No bad thing, perhaps?  But, I'm in your hands... let me know your decision!

Thanks
Andy

    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/3236b9e2/attachment.html>