Warn about unused static file scope function template declarations.

[v.g.vassilev]

Rebase + ping.

[v.g.vassilev]

@rsmith ping...

[rsmith]

lib/Sema/Sema.cpp

472–477

The comment doesn't match the code: you're removing function templates if they /do/ have specializations. And I think we should probably be walking the list of specializations and considering the template to be used if any specialization is used. That would affect a case like:

template<typename T> static void f() {}
template<> static void f<int>() {}

... where the primary template is still unused despite having a specialization.

492

Should we do the same thing for variable templates?

lib/Sema/SemaDecl.cpp

1496

Why? Defining a static operator in a header sounds like a bug to me.

[v.g.vassilev]

Iterate over the specs. Diagnose unused variable templates.

[v.g.vassilev]

Attach the right diff.

[v.g.vassilev]

lib/Sema/SemaDecl.cpp

1496

It seems we have some of these here:

include/llvm/ADT/PointerUnion.h:static bool operator==(PointerUnion<PT1, PT2> lhs, PointerUnion<PT1, PT2> rhs) {
include/llvm/ADT/PointerUnion.h:static bool operator!=(PointerUnion<PT1, PT2> lhs, PointerUnion<PT1, PT2> rhs) {
include/llvm/ADT/PointerUnion.h:static bool operator<(PointerUnion<PT1, PT2> lhs, PointerUnion<PT1, PT2> rhs) {
include/llvm/Transforms/Utils/ValueMapper.h:static inline RemapFlags operator|(RemapFlags LHS, RemapFlags RHS) {

If that's a bug, I will remove this check.

6670

@rsmith, this forces the linkage to be computed and for some invalid code such as:

namespace { struct Internal {}; }
template<typename T> __declspec(dllimport) auto InternalAutoTypeVarTmpl = Internal();

we hit an assertion in (Sema::DeduceVariableDeclarationType, SemaDecl.cpp:9991) assert(VDecl->isLinkageValid()) which assumes that the linkage wasn't computed. Should we relax/remove the assert there?

[v.g.vassilev]

Found out which is the right place to avoid the linkage computation issues.

[v.g.vassilev]

Improve comment.

[rsmith]

lib/Sema/Sema.cpp

472

neither -> none, specs -> specializations ("specs" makes me think "specifications").

503

As above.

lib/Sema/SemaDecl.cpp

1496

Yes, those are bugs.

[rsmith]

LGTM with the overloaded operator check removed.

[v.g.vassilev]

Landed in r299930.

[chapuni]

This brought warnings in llvm tree. Could you take a look, please?

http://bb.pgr.jp/builders/clang-3stage-x86_64-linux/builds/14936 (w/o asserts)

llvm/include/llvm/ADT/PointerUnion.h:193:13: warning: unused function 'operator==' [-Wunused-function]
llvm/include/llvm/ADT/PointerUnion.h:198:13: warning: unused function 'operator!=' [-Wunused-function]
llvm/include/llvm/ADT/PointerUnion.h:203:13: warning: unused function 'operator<' [-Wunused-function]
llvm/include/llvm/Analysis/LoopInfoImpl.h:581:1: warning: unused function 'addInnerLoopsToHeadersMap' [-Wunused-function]
llvm/include/llvm/ExecutionEngine/Orc/RPCUtils.h:292:14: warning: unused function 'respond' [-Wunused-function]
llvm/include/llvm/Object/ELF.h:343:13: warning: unused function 'compareAddr' [-Wunused-function]
llvm/lib/Analysis/InlineCost.cpp:1444:13: warning: unused function 'attributeMatches' [-Wunused-function]
llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp:20:14: warning: unused function 'takeObject' [-Wunused-function]
llvm/lib/IR/Verifier.cpp:836:13: warning: unused function 'isValidMetadataArrayImpl' [-Wunused-function]
llvm/lib/IR/Verifier.cpp:849:33: warning: unused function 'isValidMetadataArray' [-Wunused-function]
llvm/lib/IR/Verifier.cpp:853:33: warning: unused function 'isValidMetadataNullArray' [-Wunused-function]
llvm/lib/Object/MachOObjectFile.cpp:111:13: warning: unused function 'advance' [-Wunused-function]
llvm/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp:103:6: warning: unused function 'ReverseVector' [-Wunused-function]

[v.g.vassilev]

Thanks for pinging us. Yeah, I knew them but I was hesitant whether I should fix them or they should be fixed by the code owners. Shall I commit the fixes?

[chapuni]

I think yes, you may do. Warnings fix may be trivial as far as they are NFC.

It'd be fine that clang tree should be free from warnings with just-built clang.

[arphaman]

The operators in PointerUnion seem useful. I don't think they should be removed even if they're not used in tree, since they might be used by some out-of-tree code.

[v.g.vassilev]

@arphaman, it seems they are marked as static and this seemed like a bug to @rsmith.

[arphaman]

Ah, I see. I guess if static is removed the warning will go away. Thanks!

[mclow.lists]

The following idiom for detecting member typedefs now throws an warning:

struct __two {char __lx; char __lxx;};

namespace __has_pointer_type_imp
{
    template <class _Up> static __two __test(...);
    template <class _Up> static char __test(typename _Up::pointer* = 0);
}

template <class _Tp>
struct __has_pointer_type
    : public integral_constant<bool, sizeof(__has_pointer_type_imp::__test<_Tp>(0)) == 1>
{
};

Neither function named __test is unused; but clang now claims that they are.
Libc++ no longer builds when you have "warnings as errors" enabled.

[v.g.vassilev]

@chapuni, the LLVM warnings should be fixed in r299947.

@mclow.lists, sorry for not giving heads up :( I am working on the false positive that you reported.

[v.g.vassilev]

@mclow.lists, hm... it seems that I cannot reproduce it. It'd really help if you could paste a standalone reproducer.

[mclow.lists]

Complete reproducer:

Tested with with: clang++ -std=c++14 -Wunused-function UnusedFVassily.cpp

UnusedFVassily.cpp:8:39: warning: unused function 'test' [-Wunused-function]
template <class _Up> static two test(...);
^
UnusedFVassily.cpp:9:38: warning: unused function 'test' [-Wunused-function]
template <class _Up> static char __test(typename _Up::pointer* = 0);
^
// 2 warnings generated.

#include <type_traits>

namespace foo {

struct two {char lx; char lxx;};
namespace has_pointer_type_imp
{

template <class _Up> static __two __test(...);
template <class _Up> static char __test(typename _Up::pointer* = 0);

}

template <class _Tp>
struct __has_pointer_type

: public std::integral_constant<bool, sizeof(__has_pointer_type_imp::__test<_Tp>(0)) == 1>

{
};

}

struct S1 {};
struct S2 { typedef void *pointer; };

int main () {
static_assert (!foo::has_pointer_type<S1>::value, "" );
static_assert ( foo::has_pointer_type<S2>::value, "" );
}

[v.g.vassilev]

Reverted in r299956, due to failures like:

template <class _Up> static int __test(...);

template<typename _Tp>
auto v = __test<_Tp>(0);

[v.g.vassilev]

We agreed to implement a special switch for this, eg. -Wunused-template

[v.g.vassilev]

Implement a special diagnostic switch for the warning.

[v.g.vassilev]

@rsmith, @mclow.lists, @arphaman, I am planning to reland that soon. Let me know if you have any objections.

[v.g.vassilev]

Relanded in r302518.

[EricWF]

I think this patch still gets the following case wrong:

// foo.h
constexpr struct {
  template <class T> void operator()(T) {} // emits unused template warning
} foo;

[rsmith]

In D29877#765968, @EricWF wrote:

I think this patch still gets the following case wrong:

// foo.h
constexpr struct {
  template <class T> void operator()(T) {} // emits unused template warning
} foo;

What specifically do you think is wrong here? There is an unused internal linkage function template here. If we want to warn on unused internal linkage templates declared in headers, we should warn on this one.

Note that any use of foo here from an inline function would result in ODR violations (because you get a different foo in each translation unit), so it's probably at least a bad idea to do that. We could suppress this warning for unused internal linkage templates declared in headers, or maybe move that to a separate warning flag; can you point us at some code that does this in practice and isn't wrong?

[EricWF]

In D29877#766176, @rsmith wrote:

In D29877#765968, @EricWF wrote:

I think this patch still gets the following case wrong:

// foo.h
constexpr struct {
  template <class T> void operator()(T) {} // emits unused template warning
} foo;

What specifically do you think is wrong here? There is an unused internal linkage function template here. If we want to warn on unused internal linkage templates declared in headers, we should warn on this one.

I was confused about the linkage initially. My mistake. Should adding inline here change that?

Note that any use of foo here from an inline function would result in ODR violations (because you get a different foo in each translation unit), so it's probably at least a bad idea to do that. We could suppress this warning for unused internal linkage templates declared in headers, or maybe move that to a separate warning flag; can you point us at some code that does this in practice and isn't wrong?

No. But I can point you to range-v3 which uses this pattern and I think the idiom is somewhat appealing, but that's orthogonal to Clang diagnosing it.

[EricWF]

In D29877#766196, @EricWF wrote:

In D29877#766176, @rsmith wrote:

In D29877#765968, @EricWF wrote:

I think this patch still gets the following case wrong:

// foo.h
constexpr struct {
  template <class T> void operator()(T) {} // emits unused template warning
} foo;

What specifically do you think is wrong here? There is an unused internal linkage function template here. If we want to warn on unused internal linkage templates declared in headers, we should warn on this one.

I was confused about the linkage initially. My mistake. Should adding inline here change that?

No, it does not. Unnamed classes and their members can never have external linkage.

Note that any use of foo here from an inline function would result in ODR violations (because you get a different foo in each translation unit), so it's probably at least a bad idea to do that. We could suppress this warning for unused internal linkage templates declared in headers, or maybe move that to a separate warning flag; can you point us at some code that does this in practice and isn't wrong?

No. But I can point you to range-v3 which uses this pattern and I think the idiom is somewhat appealing, but that's orthogonal to Clang diagnosing it.

It seems like simply naming the type avoids the linkage problems. I feel silly now.

[rsmith]

In D29877#766196, @EricWF wrote:

No. But I can point you to range-v3 which uses this pattern and I think the idiom is somewhat appealing, but that's orthogonal to Clang diagnosing it.

I found this:

https://github.com/ericniebler/range-v3/blob/a4829172c0d6c43687ba213c54f430202efd7497/include/range/v3/view/zip_with.hpp#L44

This code is wrong, and creates ODR violations on lines 190 and 200.

It seems to me that the warning is firing on dangerous / broken code (yay!) but the warning is not sufficient to explain *why* the code is broken (boo!). It also seems to me that the reason why we flag up this code is not really related to the reason why this code is broken, and we should probably have a separate diagnostic for using an internal linkage entity from within an entity to which the ODR applies that is defined within a header. If we had such a diagnostic, it would seem reasonable to limit this warning to only apply to code that is *not* in a header file -- but see PR22712 for a case where even that is not quite enough.

[EricWF]

include/clang/Basic/DiagnosticGroups.td

631 ↗

(On Diff #97098)

Is libc++ not clean for this? I just ran the libc++ tests with -Wunused-template and didn't see any errors.

[v.g.vassilev]

include/clang/Basic/DiagnosticGroups.td

631 ↗

(On Diff #97098)

I think we were waiting also for the chromium guys. Perhaps they moved, too.

[v.g.vassilev]

In D29877#766301, @rsmith wrote:

In D29877#766196, @EricWF wrote:

No. But I can point you to range-v3 which uses this pattern and I think the idiom is somewhat appealing, but that's orthogonal to Clang diagnosing it.

I found this:

https://github.com/ericniebler/range-v3/blob/a4829172c0d6c43687ba213c54f430202efd7497/include/range/v3/view/zip_with.hpp#L44

This code is wrong, and creates ODR violations on lines 190 and 200.

It seems to me that the warning is firing on dangerous / broken code (yay!) but the warning is not sufficient to explain *why* the code is broken (boo!).

Perhaps we can complement this with a note, explaining the linkage situation if we fire the warning in a header file...

lib/Sema/Sema.cpp

 static bool ShouldRemoveFromUnused(Sema *SemaRef, const DeclaratorDecl *D) {
   if (D->getMostRecentDecl()->isUsed())
     return true;
 
   if (D->isExternallyVisible())
     return true;
 
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
+    // If this is a function template, we should remove if it has no
+    // specializations.
+    if (FunctionTemplateDecl *Template = FD->getDescribedFunctionTemplate()) {
+      if (std::distance(Template->spec_begin(), Template->spec_end()))
+        return true;
+    }
+
     // UnusedFileScopedDecls stores the first declaration.
     // The declaration may have become definition so check again.
     const FunctionDecl *DeclToCheck;
     if (FD->hasBody(DeclToCheck))
       return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
 
     // Later redecls may add new information resulting in not having to warn,
     // so check again.
     DeclToCheck = FD->getMostRecentDecl();
     if (DeclToCheck != FD)
       return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
   }
 
   if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     // If a variable usable in constant expressions is referenced,
     // don't warn if it isn't used: if the value of a variable is required
     // for the computation of a constant expression, it doesn't make sense to
     // warn even if the variable isn't odr-used.  (isReferenced doesn't
     // precisely reflect that, but it's a decent approximation.)
     if (VD->isReferenced() &&
         VD->isUsableInConstantExpressions(SemaRef->Context))
       return true;
 
     // UnusedFileScopedDecls stores the first declaration.
     // The declaration may have become definition so check again.
     const VarDecl *DeclToCheck = VD->getDefinition();
     if (DeclToCheck)
       return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
 
     // Later redecls may add new information resulting in not having to warn,
     // so check again.
     DeclToCheck = VD->getMostRecentDecl();
     if (DeclToCheck != VD)

lib/Sema/SemaDecl.cpp

if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
 
     if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD)) {
       if (MD->isVirtual() || IsDisallowedCopyOrAssign(MD))
         return false;
     } else {
       // 'static inline' functions are defined in headers; don't warn.
       if (FD->isInlined() && !isMainFileLoc(*this, FD->getLocation()))
         return false;
+      // 'static operator' functions are defined in headers; don't warn.
+      if (FD->isOverloadedOperator() &&
+          !isMainFileLoc(*this, FD->getLocation()))
+        return false;
     }
 
     if (FD->doesThisDeclarationHaveABody() &&
         Context.DeclMustBeEmitted(FD))
       return false;
   } else if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     // Constants and utility variables are defined in headers with internal
     // linkage; don't warn.  (Unlike functions, there isn't a convenient marker
if (D.isRedeclaration() && !Previous.empty()) {
     checkDLLAttributeRedeclaration(
         *this, dyn_cast<NamedDecl>(Previous.getRepresentativeDecl()), NewVD,
         IsMemberSpecialization, D.isFunctionDefinition());
   }
 
   if (NewTemplate) {
     if (NewVD->isInvalidDecl())
       NewTemplate->setInvalidDecl();
     ActOnDocumentableDecl(NewTemplate);
     return NewTemplate;
   }
 
   return NewVD;
 }
 
 /// Enum describing the %select options in diag::warn_decl_shadow.
 enum ShadowedDeclKind { SDK_Local, SDK_Global, SDK_StaticMember, SDK_Field };
if (!getLangOpts().CUDAAllowVariadicFunctions && NewFD->isVariadic() &&
       Diag(NewFD->getLocation(), diag::err_variadic_device_fn);
     }
   }
 
   if (getLangOpts().CPlusPlus) {
     if (FunctionTemplate) {
       if (NewFD->isInvalidDecl())
         FunctionTemplate->setInvalidDecl();
+      MarkUnusedFileScopedDecl(NewFD);
       return FunctionTemplate;
     }
   }
 
   if (NewFD->hasAttr<OpenCLKernelAttr>()) {
     // OpenCL v1.2 s6.8 static is invalid for kernel functions.
     if ((getLangOpts().OpenCLVersion >= 120)
         && (SC == SC_Static)) {

test/SemaCXX/warn-unused-filescoped.cpp

namespace test7
 
   template<typename U>
   inline void bar(int, U) { }
 
   template<>
   inline void bar(int, int) { }
 };
 
+namespace test8 {
+// Should ignore overloaded operators.
+template <typename PT1, typename PT2>
+struct S {};
+template <typename PT1, typename PT2>
+static bool operator==(S<PT1, PT2> lhs, S<PT1, PT2> rhs) { }
+}
+
 namespace pr19713 {
 #if __cplusplus >= 201103L
   static constexpr int constexpr1() { return 1; }
   constexpr int constexpr2() { return 2; }
 #endif
 }
 
 #else

 }
 
 namespace test8 {
 static void func();
 void bar() { void func() __attribute__((used)); }
 static void func() {}
 }
 
+namespace test9 {
+template<typename T>
+static void completeRedeclChainForTemplateSpecialization() { } // expected-warning {{unused}}
+}
+
 namespace pr19713 {
 #if __cplusplus >= 201103L
   // FIXME: We should warn on both of these.
   static constexpr int constexpr3() { return 1; } // expected-warning {{unused}}
   constexpr int constexpr4() { return 2; }
 #endif
 }
 
 #endif