<div dir="ltr">On Tue, Mar 19, 2013 at 3:49 PM, Alexey Samsonov <span dir="ltr"><<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><span style="color:rgb(80,0,80)">On Tue, Mar 19, 2013 at 8:12 PM, David Blaikie </span><span dir="ltr" style="color:rgb(80,0,80)"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span><span style="color:rgb(80,0,80)"> wrote:</span><br>
<div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On Tue, Mar 19, 2013 at 3:11 AM, Alexey Samsonov <<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>> wrote:<br>
><br>
> On Fri, Mar 15, 2013 at 11:07 AM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>><br>
> wrote:<br>
>><br>
>><br>
>><br>
>><br>
>> On Thu, Mar 14, 2013 at 11:21 PM, Alexey Samsonov <<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>><br>
>> wrote:<br>
>>><br>
>>><br>
>>> On Fri, Mar 15, 2013 at 4:20 AM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>> Author: echristo<br>
>>>> Date: Thu Mar 14 19:20:17 2013<br>
>>>> New Revision: 177132<br>
>>>><br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=177132&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=177132&view=rev</a><br>
>>>> Log:<br>
>>>> Fix a virtual destructor warning.<br>
>>>><br>
>>>> Patch by Manuel Klimek!<br>
>>><br>
>>><br>
>>> Thanks! Should we make this warning fire in CMake build +<br>
>>> LLVM_ENABLE_WERROR?<br>
>>><br>
>><br>
>><br>
>> Mmm. Probably.<br>
><br>
><br>
> Done in r177385.<br>
<br>
</div>Sorry I didn't reply earlier - but just to check, do we want this?<br>
<br>
Clang has a warning about virtual destruction as well that might be<br>
sufficiently accurate without having to provide virtual dtors for<br>
types that are used, but not owned, virtually. Hmm, seems that's off<br>
by default too.<br>
<br>
So, alternate proposal:<br>
<br>
disable GCC's -Wnon-virtual-dtor<br>
enable Clang's -Wdelete-non-virtual-dtor<br>
<br>
It catches things like:<br>
<br>
struct foo {<br>
virtual void bar();<br>
};<br>
<br>
struct derived: foo { };<br>
<br>
int main() {<br>
foo *f = new derived;<br>
delete f;<br>
}<br>
<br>
(without incorrectly warning on: void func(foo& f) { f.bar(); } int<br>
main() { derived d; func(d); })<br>
<br>
Which could still be a false positive if such a pointer doesn't<br>
actually point to various derived objects at runtime, but is probably<br>
still a better signal than -Wnon-virtual-dtor.<br></blockquote><div><br></div></div></div><div><div>FTR, -Wnon-virtual-dtor didn't show any warnings on LLVM/Clang code, and</div><div>3 (seemingly not true positive) warnings on lld code (which I silenced).</div>
<div><br></div></div><div>Just to make sure we're on the same page - do you want</div><div>to add "-Wno-non-virtual-dtor -Wdelete-non-virtual-dtor" to compile flags?</div><div><br></div><div>
This sounds fine to me, and it may indeed help with false positives, but<br></div><div>my motivation for the change was patch by Manuel - it seems that some people</div><div>actually compile LLVM codebase with -Wnon-virtual-dtor and expect it</div>
<div>to compile w/o warnings.</div></div></div></div></blockquote><div><br></div><div>I just caught one of these warnings in <a href="http://llvm-reviews.chandlerc.com/D1170">http://llvm-reviews.chandlerc.com/D1170</a> before committing. I have a class (StringSaver) that I never intend to heap allocate or delete from a base pointer, but -Wnon-virtual-dtor fires.</div>
<div><br></div><div>Maybe we should consider changing -Wnon-virtual-dtor to "-Wdelete-non-virtual-dtor -Wno-non-virtual-dtor". Would that break anyone?</div></div></div></div>