[llvm] r206727 - Protect the ArgList dtor

[David Blaikie]

On Mon, Apr 21, 2014 at 1:22 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>> On 21.04.2014, at 02:07, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> Author: dblaikie
>> Date: Sun Apr 20 18:59:00 2014
>> New Revision: 206727
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=206727&view=rev
>> Log:
>> Protect the ArgList dtor
>>
>> It could even be made non-virtual if it weren't for bad compiler
>> warnings.
>
> Do we still support compilers with a broken non-virtual dtor warning?
> GCC fixed this in 4.3 or 4.4.

Looks like in every version I have (4.6 to ToT) the warning is better
- it only flags if the ctor is "accessible" (public, by the looks of
it) and the warning is off by default (not even under -Wall or
-Wextra). Clang's warning isn't this smart. Though the warning is off
by default in GCC and Clang

In 4.7 and above there's "-Wdelete-non-virtual-dtor" which is enabled
by -Wall and warns at the delete which is more accurate.

So long as we're dynamically allocating ArgLists, we'd either need to
mark the concrete ArgLists as final (this correctly suppresses
-Wdelete-non-virtual-dtor in GCC 4.7 and Clang)

Bumped the original post commit review of r177385 which enabled this
warning in the first place.

>
> - Ben
>>
>> This demonstrates that ArgList objects aren't destroyed polymorphically
>> and possibly that they aren't even used polymorphically. If that's the
>> case, it might be possible to refactor the two ArgList types more
>> separately and simplify the Arg ownership model. *continues
>> experimenting*
>>
>> Modified:
>>    llvm/trunk/include/llvm/Option/ArgList.h
>>    llvm/trunk/lib/Option/ArgList.cpp
>>
>> Modified: llvm/trunk/include/llvm/Option/ArgList.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Option/ArgList.h?rev=206727&r1=206726&r2=206727&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Option/ArgList.h (original)
>> +++ llvm/trunk/include/llvm/Option/ArgList.h Sun Apr 20 18:59:00 2014
>> @@ -106,10 +106,14 @@ private:
>>   arglist_type Args;
>>
>> protected:
>> -  ArgList();
>> +  // Default ctor provided explicitly as it is not provided implicitly due to
>> +  // the presence of the (deleted) copy ctor above.
>> +  ArgList() { }
>> +  // Virtual to provide a vtable anchor and because -Wnon-virtua-dtor warns, not
>> +  // because this type is ever actually destroyed polymorphically.
>> +  virtual ~ArgList();
>>
>> public:
>> -  virtual ~ArgList();
>>
>>   /// @name Arg Access
>>   /// @{
>>
>> Modified: llvm/trunk/lib/Option/ArgList.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Option/ArgList.cpp?rev=206727&r1=206726&r2=206727&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Option/ArgList.cpp (original)
>> +++ llvm/trunk/lib/Option/ArgList.cpp Sun Apr 20 18:59:00 2014
>> @@ -33,11 +33,6 @@ void arg_iterator::SkipToNextArg() {
>>   }
>> }
>>
>> -//
>> -
>> -ArgList::ArgList() {
>> -}
>> -
>> ArgList::~ArgList() {
>> }
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits