Details
- Reviewers
rsmith
Diff Detail
Event Timeline
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. |
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) { 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? |
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.
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);
@rsmith, @mclow.lists, @arphaman, I am planning to reland that soon. Let me know if you have any objections.
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 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.
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.
I found this:
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.
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. |
include/clang/Basic/DiagnosticGroups.td | ||
---|---|---|
631 | I think we were waiting also for the chromium guys. Perhaps they moved, too. |
Perhaps we can complement this with a note, explaining the linkage situation if we fire the warning in a header file...
Is libc++ not clean for this? I just ran the libc++ tests with -Wunused-template and didn't see any errors.