llvm dev - Dec 2011 - [LLVMdev] anchoring explicit template instantiations

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.

If you take a look at CommandLine.h/CommandLine.cpp you'll see some
code that basically amounts to this:

header:

    template<class DataType>
    class basic_parser {
      virtual ~basic_parser() {}
    };

    __extension__ extern template class basic_parser<bool>;

implementation:

    template class basic_parser<bool>;

(both lines are wrapped in a macro (Compiler.h:77-88) & are no-ops in
non-GNUC compilers (where the __extension__ extern is not available))

Adding in a virtual anchor function with an out-of-line (but still
template) definition (either in the header or the cpp file) does not
remove the warning.

So the question is - is there any way to anchor these explicit
instantiations? Should the warning (& possibly even the underlying
implementation/codegen) be fixed to not flag this particular case of
the GNUC extension - since these vtables should be able to be anchored
(with the addition of such an out of line definition - either in the
header or cpp file (though in this case I don't think it should be
necessary in the header - since only these explicit instantiations of
basic_parser are used))? Is there a portable way to address the
warning? If not, should the warning just be silent, or have a separate
group/warning for this case so the actionable warning can remain while
this one can be disabled?

Thanks,
- David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: anchors.diff
Type: text/x-diff
Size: 3491 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111129/4d91afe0/attachment.diff>

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.

-Chris
> 
> If you take a look at CommandLine.h/CommandLine.cpp you'll see some
> code that basically amounts to this:
> 
> header:
> 
>    template<class DataType>
>    class basic_parser {
>      virtual ~basic_parser() {}
>    };
> 
>    __extension__ extern template class basic_parser<bool>;
> 
> implementation:
> 
>    template class basic_parser<bool>;
> 
> (both lines are wrapped in a macro (Compiler.h:77-88) & are no-ops in
> non-GNUC compilers (where the __extension__ extern is not available))
> 
> Adding in a virtual anchor function with an out-of-line (but still
> template) definition (either in the header or the cpp file) does not
> remove the warning.
> 
> So the question is - is there any way to anchor these explicit
> instantiations? Should the warning (& possibly even the underlying
> implementation/codegen) be fixed to not flag this particular case of
> the GNUC extension - since these vtables should be able to be anchored
> (with the addition of such an out of line definition - either in the
> header or cpp file (though in this case I don't think it should be
> necessary in the header - since only these explicit instantiations of
> basic_parser are used))? Is there a portable way to address the
> warning? If not, should the warning just be silent, or have a separate
> group/warning for this case so the actionable warning can remain while
> this one can be disabled?
> 
> Thanks,
> - David
> <anchors.diff>_______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

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?

(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!");
   }


and

--- a/lib/Support/APInt.cpp
+++ 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;
-  else if (this->ule(nextSquare)) {
+  if (this->ule(nextSquare)) {
     APInt midpoint((nextSquare - square).udiv(two));
     APInt offset(*this - square);
     if (offset.ult(midpoint))
       return x_old;
-    else
-      return x_old + 1;
-  } else
-    llvm_unreachable("Error in APInt::sqrt computation");
-  return x_old + 1;
+    return x_old + 1;
+  }
+  llvm_unreachable("Error in APInt::sqrt computation");
 }

(a return after an llvm_unreachable - muddied up with a bit of
syntactic tidyup))

& I'll take the detailed discussion about -Wweak-vtables for template
instantiations to cfe-dev & see what they have to say.

- David