<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META http-equiv=Content-Type content="text/html; charset=utf-8">
<META content="MSHTML 6.00.2900.6400" name=GENERATOR>
<STYLE></STYLE>
</HEAD>
<BODY bgColor=#ffffff>
<DIV>On Thursday, December 03, 2015 6:06 PM, David Blaikie wrote:</DIV>
<BLOCKQUOTE dir=ltr
style="PADDING-RIGHT: 0px; PADDING-LEFT: 5px; MARGIN-LEFT: 5px; BORDER-LEFT: #000000 2px solid; MARGIN-RIGHT: 0px">
<DIV style="FONT: 10pt arial">On Thu, Dec 3, 2015 at 9:00 AM, Andy Gibbs
wrote:<BR></DIV>
<DIV dir=ltr>
<DIV class=gmail_quote>
<BLOCKQUOTE class=gmail_quote
style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid">On
Thursday, December 03, 2015 5:29 PM, David Blaikie wrote:<SPAN
class=""><BR>On Thu, Dec 3, 2015 at 12:20 AM, Andy Gibbs wrote:<BR>
<BLOCKQUOTE class=gmail_quote
style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid">
<BLOCKQUOTE class=gmail_quote
style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid">Author:
andyg<BR>Date: Thu Dec 3 02:20:20 2015<BR>New Revision:
254592<BR><BR>URL: <A
href="http://llvm.org/viewvc/llvm-project?rev=254592&view=rev"
target=_blank
rel=noreferrer>http://llvm.org/viewvc/llvm-project?rev=254592&view=rev</A><BR>Log:<BR>Fix
class SCEVPredicate has virtual functions and accessible non-virtual
destructor.<BR><BR></BLOCKQUOTE><BR>Wait - why did the dtor need to be
made virtual at all? These objects are never<BR>destroyed polymorphically,
so far as I can tell.<BR><BR>Could we just disable the GCC warning that
was firing here instead? (if I<BR>understand correctly and this was a GCC
warning)<BR></BLOCKQUOTE><BR></SPAN>Fine, but IMHO the warning is a valid
one. The code may not currently destroy these<BR>objects
polymorphically, but if that changes in future (for whatever reason)
then<BR>its a potential memory leak. Its one of those "golden rule"
moments, isn't it? :o)<BR></BLOCKQUOTE>
<DIV><BR></DIV>
<DIV>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.<BR><BR>Both Clang and GCC don't warn in
that case - but GCC does warn if that class then has any friends.<BR><BR>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.<BR><BR>Not quite
sure what golden rule(s) you're referring to.</DIV>
<DIV><FONT size=2></FONT> </DIV></DIV></DIV></BLOCKQUOTE>
<DIV class=gmail_extra dir=ltr><FONT size=2>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.</FONT></DIV>
<DIV class=gmail_extra dir=ltr><FONT size=2></FONT> </DIV>
<DIV class=gmail_extra dir=ltr><FONT size=2>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)</FONT></DIV>
<DIV class=gmail_extra dir=ltr><FONT size=2></FONT> </DIV>
<DIV class=gmail_extra dir=ltr><FONT size=2>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!</FONT></DIV>
<DIV class=gmail_extra dir=ltr><FONT size=2></FONT> </DIV>
<DIV class=gmail_extra dir=ltr><FONT size=2>Thanks</FONT></DIV>
<DIV class=gmail_extra dir=ltr><FONT size=2>Andy</FONT></DIV>
<BLOCKQUOTE dir=ltr
style="PADDING-RIGHT: 0px; PADDING-LEFT: 5px; MARGIN-LEFT: 5px; BORDER-LEFT: #000000 2px solid; MARGIN-RIGHT: 0px">
<DIV class=gmail_quote dir=ltr>
<BLOCKQUOTE class=gmail_quote
style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid"><BR>Rather
than disable the warning, which could open up potential problems
elsewhere<BR>not even related to this class, </BLOCKQUOTE>
<DIV><BR></DIV>
<DIV>Except that it's highly restricted because the dtor is protected and the
derived classes are final.</DIV>
<DIV> </DIV>
<BLOCKQUOTE class=gmail_quote
style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid">I
think it is better to fix the issue, even if it is<BR>technically/arguably a
non-issue in this case.<BR><BR>Its not like there is a huge overhead (code
size, execution time, ...) to the fix.<BR>The classes already have vtables
due to the other virtual methods, for example.<BR></BLOCKQUOTE>
<DIV><BR></DIV>
<DIV>But we shouldn't be adding features just because of a compiler warning -
we should add features if/when we need them.<BR><BR>-
Dave</DIV></DIV></BLOCKQUOTE></BODY></HTML>