Page MenuHomePhabricator

Warn about unused static file scope function template declarations.
ClosedPublic

Authored by v.g.vassilev on Feb 12 2017, 1:36 PM.

Details

Reviewers
rsmith

Diff Detail

Event Timeline

v.g.vassilev created this revision.Feb 12 2017, 1:36 PM

Rebase + ping.

rsmith added inline comments.Mar 13 2017, 1:55 PM
lib/Sema/Sema.cpp
466–471

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.

486

Should we do the same thing for variable templates?

lib/Sema/SemaDecl.cpp
1499

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

v.g.vassilev marked 2 inline comments as done.

Iterate over the specs. Diagnose unused variable templates.

Attach the right diff.

v.g.vassilev added inline comments.Apr 5 2017, 3:27 PM
lib/Sema/SemaDecl.cpp
1499

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.

6692

@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?

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

v.g.vassilev marked an inline comment as done.

Improve comment.

rsmith added inline comments.Apr 10 2017, 2:19 PM
lib/Sema/Sema.cpp
466

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

497

As above.

lib/Sema/SemaDecl.cpp
1499

Yes, those are bugs.

rsmith accepted this revision.Apr 10 2017, 2:19 PM

LGTM with the overloaded operator check removed.

This revision is now accepted and ready to land.Apr 10 2017, 2:19 PM
v.g.vassilev closed this revision.Apr 11 2017, 3:27 AM
v.g.vassilev marked 5 inline comments as done.

Landed in r299930.

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]

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?

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.

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.

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

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

mclow.lists added a subscriber: mclow.lists.EditedApr 11 2017, 8:07 AM

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.

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

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

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

Reverted in r299956, due to failures like:

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

template<typename _Tp>
auto v = __test<_Tp>(0);
v.g.vassilev reopened this revision.Apr 12 2017, 2:32 PM
This revision is now accepted and ready to land.Apr 12 2017, 2:32 PM
v.g.vassilev planned changes to this revision.Apr 12 2017, 2:44 PM

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

Implement a special diagnostic switch for the warning.

This revision is now accepted and ready to land.Apr 28 2017, 7:51 AM

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

v.g.vassilev closed this revision.May 9 2017, 4:39 AM

Relanded in r302518.

EricWF added a subscriber: EricWF.May 26 2017, 11:48 AM

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;

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?

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.

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.

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 added inline comments.May 26 2017, 5:10 PM
include/clang/Basic/DiagnosticGroups.td
631

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

v.g.vassilev added inline comments.May 27 2017, 2:30 AM
include/clang/Basic/DiagnosticGroups.td
631

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

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