<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 3, 2015 at 9:32 AM, Andy Gibbs <span dir="ltr"><<a href="mailto:andyg1001@hotmail.co.uk" target="_blank">andyg1001@hotmail.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><u></u>
<div bgcolor="#ffffff">
<div>On Thursday, December 03, 2015 6:06 PM, David Blaikie wrote:</div><span>
<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><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" rel="noreferrer" target="_blank">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>
</span><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></blockquote><div><br></div><div>We still have Clang that (arguably) correctly implements the warning without the friend exception that GCC implements.<br><br>My basic bar would be: Either we consider GCC's behavior correct and we fix Clang's warning to do the same (& keep your code change). Or we consider GCC's behavior to be incorrect and we undo this change and disable GCC's warning.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#ffffff">
<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" </font></div></div></blockquote><div><br></div><div>Well, it adds the feature of polymorphic destruction for this type/hierarchy.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#ffffff"><div class="gmail_extra" dir="ltr"><font size="2">and there is (unless you can correct
me here) no tangible overhead,</font></div></div></blockquote><div><br></div><div>dynamic dispatch running the dtor is something, though it's unlikely to be significant for this class<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#ffffff"><div class="gmail_extra" dir="ltr"><font size="2"> just additional safety. No bad thing,
perhaps? But, I'm in your hands... let me know your decision!</font></div></div></blockquote><div><br></div><div>My usual reason to push back against changes like this is the precedent - I don't want us to keep making sub-optimal changes to placate a warning. Death by a thousand cuts & all that (though there's probably only tens of cases of non-virtual dtors in polymorphic hierarchies across LLVM and Clang, to be fair) </div><div><br></div><div>I'd rather discuss these things & come to an understanding/agreement, but if a request is simpler, I think this change should be reverted and, if you'd like to keep the GCC build warning-clean, a patch to disable this warning would be acceptable. Clang's warnings will continue to catch the other cases & self-hosting buildbots will flag them soon enough.<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#ffffff">
<div class="gmail_extra" dir="ltr"><font size="2"></font> </div>
<div class="gmail_extra" dir="ltr"><font size="2">Thanks</font></div><span><font color="#888888">
<div class="gmail_extra" dir="ltr"><font size="2">Andy</font></div></font></span><span>
<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></span></div>
</blockquote></div><br></div></div>