[compiler-rt] r177132 - Fix a virtual destructor warning.

[David Blaikie]

On Thu, Jul 18, 2013 at 9:29 AM, Reid Kleckner <rnk at google.com> wrote:
> On Tue, Mar 19, 2013 at 3:49 PM, Alexey Samsonov <samsonov at google.com>
> wrote:
>>
>> On Tue, Mar 19, 2013 at 8:12 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> On Tue, Mar 19, 2013 at 3:11 AM, Alexey Samsonov <samsonov at google.com>
>>> wrote:
>>> >
>>> > On Fri, Mar 15, 2013 at 11:07 AM, Eric Christopher <echristo at gmail.com>
>>> > wrote:
>>> >>
>>> >>
>>> >>
>>> >>
>>> >> On Thu, Mar 14, 2013 at 11:21 PM, Alexey Samsonov
>>> >> <samsonov at google.com>
>>> >> wrote:
>>> >>>
>>> >>>
>>> >>> On Fri, Mar 15, 2013 at 4:20 AM, Eric Christopher
>>> >>> <echristo at gmail.com>
>>> >>> wrote:
>>> >>>>
>>> >>>> Author: echristo
>>> >>>> Date: Thu Mar 14 19:20:17 2013
>>> >>>> New Revision: 177132
>>> >>>>
>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=177132&view=rev
>>> >>>> Log:
>>> >>>> Fix a virtual destructor warning.
>>> >>>>
>>> >>>> Patch by Manuel Klimek!
>>> >>>
>>> >>>
>>> >>> Thanks! Should we make this warning fire in CMake build +
>>> >>> LLVM_ENABLE_WERROR?
>>> >>>
>>> >>
>>> >>
>>> >> Mmm. Probably.
>>> >
>>> >
>>> > Done in r177385.
>>>
>>> Sorry I didn't reply earlier - but just to check, do we want this?
>>>
>>> Clang has a warning about virtual destruction as well that might be
>>> sufficiently accurate without having to provide virtual dtors for
>>> types that are used, but not owned, virtually. Hmm, seems that's off
>>> by default too.
>>>
>>> So, alternate proposal:
>>>
>>> disable GCC's -Wnon-virtual-dtor
>>> enable Clang's -Wdelete-non-virtual-dtor
>>>
>>> It catches things like:
>>>
>>> struct foo {
>>>   virtual void bar();
>>> };
>>>
>>> struct derived: foo { };
>>>
>>> int main() {
>>>   foo *f = new derived;
>>>   delete f;
>>> }
>>>
>>> (without incorrectly warning on: void func(foo& f) { f.bar(); } int
>>> main() { derived d; func(d); })
>>>
>>> Which could still be a false positive if such a pointer doesn't
>>> actually point to various derived objects at runtime, but is probably
>>> still a better signal than -Wnon-virtual-dtor.
>>
>>
>> FTR, -Wnon-virtual-dtor didn't show any warnings on LLVM/Clang code, and
>> 3 (seemingly not true positive) warnings on lld code (which I silenced).
>>
>> Just to make sure we're on the same page - do you want
>> to add "-Wno-non-virtual-dtor -Wdelete-non-virtual-dtor" to compile flags?
>>
>> This sounds fine to me, and it may indeed help with false positives, but
>> my motivation for the change was patch by Manuel - it seems that some
>> people
>> actually compile LLVM codebase with -Wnon-virtual-dtor and expect it
>> to compile w/o warnings.
>
>
> I just caught one of these warnings in
> http://llvm-reviews.chandlerc.com/D1170 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.

Bumping this because I hit this again in the ArgList API. ArgLists are
never owned polymorphically (as demonstrated by the dtor being
protected), but are used polymorphically.

If we improved Clang's warning to match GCC's we could make the dtor
non-virtual so long as it's protected, which would be something. But
if we could disable both GCC and Clang's -Wnon-virtual-dtor warning we
could remove the dtor entirely and just get an error if the type was
ever destroyed polymorphically.

> Maybe we should consider changing -Wnon-virtual-dtor to
> "-Wdelete-non-virtual-dtor -Wno-non-virtual-dtor".  Would that break anyone?

Rumor has it it would break Google, though I'd like to figure out how
to address that.