[cfe-dev] [LLVMdev] anchoring explicit template instantiations

[David Blaikie]

On Thu, Dec 1, 2011 at 9:11 AM, Chris Lattner <clattner at apple.com> wrote:
>
> On Dec 1, 2011, at 12:08 AM, David Blaikie wrote:
>
>> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <clattner at apple.com> wrote:
>>> On Nov 29, 2011, at 12:26 AM, David Blaikie wrote:
>>>> For a bit of an experiment I've been trying to compile LLVM & Clang
>>>> with -Weverything (disabling any errors that seem like more noise/less
>>>> interesting). One warning I've recently hit a few instances of is
>>>> -Wweak-vtable which is, in fact, an explicitly documented LLVM coding
>>>> standard ( http://llvm.org/docs/CodingStandards.html#ll_virtual_anch
>>>> ). Some instances of this have been easy to fix (see attached patch -
>>>> if it looks good to anyone I'll check that in) but a particular set of
>>>> them have been a little more problematic.
>>>
>>> Nice, please commit your patch.  I don't know about explicit instantiations though, maybe a C++ guru on the clang list knows.
>>
>> Thanks Chris, committed as r145578. I don't suppose you'll mind some
>> similar commits as I encounter them?
>
> Yep, please feel free.

While you said this - given that I've now gone & fixed /every/
violation of -Wweak-vtables across LLVM & Clang (apart from some llvm
target tblgen problems - not sure how practical they are to fix. And
gtest also fires this warning) I thought I should probably get at
least a '*nod*' before I check this in.

Actually it'd be kind of interesting to see how much/if any difference
in compile time this actually makes. I do wonder.

(also, I implemented this by adding private anchors, like my original
version - this does actually have a difference at runtime, of course -
since now each of these types has another entry in their vtable.
Alternatively I could've used their destructors (but then they
wouldn't be inline anymore - but probably most of them are called
virtually anyway so their inline-ness doesn't matter). Also I had to
add about 10 source files in total as implementation files for
header-only cases that needed anchors)

Also - do we have anywhere we could put standard flags that LLVM
should successfully compile with? At least if clang is being used,
perhaps? So we could add -Wweak-vtables -Werror for this case & add
more as we deem it appropriate.

- David

>> (there's also some legitimate unreachable code warnings I'd be happy
>> to fix as I find them, things like:
>>
>> --- a/lib/Support/CommandLine.cpp
>> +++ b/lib/Support/CommandLine.cpp
>> @@ -294,10 +294,7 @@ static inline bool ProvideOption(Option *Handler,
>> StringRef ArgName,
>>     break;
>>
>>   default:
>> -    errs() << ProgramName
>> -         << ": Bad ValueMask flag! CommandLine usage error:"
>> -         << Handler->getValueExpectedFlag() << "\n";
>> -    llvm_unreachable(0);
>> +    llvm_unreachable("Bad ValueMask flag!");
>
> This patch would lose the expected flag value, which is unfortunate.
>
>> +++ b/lib/Support/APInt.cpp
>> @@ -1440,16 +1440,14 @@ APInt APInt::sqrt() const {
>>   APInt nextSquare((x_old + 1) * (x_old +1));
>>   if (this->ult(square))
>>     return x_old;
>> +  if (this->ule(nextSquare)) {
>>     APInt midpoint((nextSquare - square).udiv(two));
>>     APInt offset(*this - square);
>>     if (offset.ult(midpoint))
>>       return x_old;
>> +  }
>> +  llvm_unreachable("Error in APInt::sqrt computation");
>> }
>>
>> (a return after an llvm_unreachable - muddied up with a bit of
>> syntactic tidyup))
>
> This is an abuse of llvm_unreachable.  It should just replace "if ule" check with:
>
> assert(this->ule(nextSquare) && "Error in APInt::sqrt computation");
>
>> & I'll take the detailed discussion about -Wweak-vtables for template
>> instantiations to cfe-dev & see what they have to say.
>
> Thanks David!
>
> -Chris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-weak-vtables.diff
Type: text/x-diff
Size: 77820 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111210/b7de31d0/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-weak-vtables.diff
Type: text/x-diff
Size: 106686 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111210/b7de31d0/attachment-0001.diff>