<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Mar 19, 2013 at 8:12 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<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 class="im">On Tue, Mar 19, 2013 at 3:11 AM, Alexey Samsonov <<a href="mailto:samsonov@google.com">samsonov@google.com</a>> wrote:<br>

><br>
> On Fri, Mar 15, 2013 at 11:07 AM, Eric Christopher <<a href="mailto:echristo@gmail.com">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">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">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 style><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 style>Just to make sure we're on the same page - do you want</div><div style>to add "-Wno-non-virtual-dtor -Wdelete-non-virtual-dtor" to compile flags?</div><div style><br></div><div style>
This sounds fine to me, and it may indeed help with false positives, but<br></div><div style>my motivation for the change was patch by Manuel - it seems that some people</div><div style>actually compile LLVM codebase with -Wnon-virtual-dtor and expect it</div>
<div style>to compile w/o warnings.</div><div> </div><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 class=""><div class="h5"><br>
><br>
>><br>
>><br>
>> -eric<br>
>><br>
>>>><br>
>>>><br>
>>>> Modified:<br>
>>>>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc<br>
>>>>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h<br>
>>>><br>
>>>> Modified:<br>
>>>> compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc<br>
>>>> URL:<br>
>>>> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc?rev=177132&r1=177131&r2=177132&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc?rev=177132&r1=177131&r2=177132&view=diff</a><br>

>>>><br>
>>>> ==============================================================================<br>
>>>> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc<br>
>>>> (original)<br>
>>>> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc<br>
>>>> Thu Mar 14 19:20:17 2013<br>
>>>> @@ -22,6 +22,8 @@ ThreadContextBase::ThreadContextBase(u32<br>
>>>>    name[0] = '\0';<br>
>>>>  }<br>
>>>><br>
>>>> +ThreadContextBase::~ThreadContextBase() {}<br>
>>>> +<br>
>>>>  void ThreadContextBase::SetName(const char *new_name) {<br>
>>>>    name[0] = '\0';<br>
>>>>    if (new_name) {<br>
>>>><br>
>>>> Modified:<br>
>>>> compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h<br>
>>>> URL:<br>
>>>> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h?rev=177132&r1=177131&r2=177132&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h?rev=177132&r1=177131&r2=177132&view=diff</a><br>

>>>><br>
>>>> ==============================================================================<br>
>>>> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h<br>
>>>> (original)<br>
>>>> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h<br>
>>>> Thu Mar 14 19:20:17 2013<br>
>>>> @@ -34,6 +34,7 @@ enum ThreadStatus {<br>
>>>>  class ThreadContextBase {<br>
>>>>   public:<br>
>>>>    explicit ThreadContextBase(u32 tid);<br>
>>>> +  virtual ~ThreadContextBase();<br>
>>>><br>
>>>>    const u32 tid;  // Thread ID. Main thread should have tid = 0.<br>
>>>>    u64 unique_id;  // Unique thread ID.<br>
>>>><br>
>>>><br>
>>>> _______________________________________________<br>
>>>> llvm-commits mailing list<br>
>>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> --<br>
>>> Alexey Samsonov, MSK<br>
>><br>
>><br>
><br>
><br>
><br>
> --<br>
> Alexey Samsonov, MSK<br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div>
</div></div>