[LLVMdev] anchoring explicit template instantiations

[Chris Lattner]

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.

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