[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.

Thanks - I'll see what comes up.

>> (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.

It is unfortunate - though I'm not sure whether you're suggesting that
the change should be made anyway, or not.

I'd say the whole unreachable could be dropped & an assertion placed
inside getValueExpectedFlag which is where the actual unsafe bit->enum
conversion is occuring.
(that would still leave the need for a default in this switch because
one of the values of the enum isn't covered - ValueMask, but really
that probably shouldn't be one of the enumeration values anyway,
should it? (looks like it should just be a hidden constant somewhere
in the implementation details of llvm::Option))

>> +++ 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:

Ah, so it is. I hadn't noticed (was just looking at the simple
transformation without actually parsing the semantics any further).

Committed with assert instead of abusive unreachable in r145627

Thanks,
- David