Make the -Wunused-template default.

[v.g.vassilev]

Summary

https://reviews.llvm.org/D29877 implements a useful -Wunused-template diagnostic detects unused internal linkage templates. This helps finding potential ODR issues in headers.

Many years ago we made the diagnostic optional because libcxx was not ready. I am hoping @ldionne to help with that if we have not cleaned up libcxx already.

[aaron.ballman]

Precommit CI shows issues with the libc++ tests that need to be addressed. This should also come with a release note.

clang/test/SemaCXX/warn-func-not-needed.cpp

13

Why is this unused? f<long>() in foo() should cause this to be used, right?

How should a user silence this diagnostic without disabling it entirely?

[philnik]

The emitted warnings from the libc++ CI look like a false-positive to me. While the functions are never called, they are used in an unevaluated context. I would expect -Wunused warnings to only be emitted when I can just remove the code without problems, which doesn't seem to be the case here. It would probably just get turned off again by lots of people if there are too many false-positives, which I don't think is the goal here.

[v.g.vassilev]

In D143524#4148006, @philnik wrote:

The emitted warnings from the libc++ CI look like a false-positive to me. While the functions are never called, they are used in an unevaluated context. I would expect -Wunused warnings to only be emitted when I can just remove the code without problems, which doesn't seem to be the case here. It would probably just get turned off again by lots of people if there are too many false-positives, which I don't think is the goal here.

From what I see is that most of the templates are marked with static which means internal linkage. Entities with internal linkage in header files are essentially different across translation units which is an ODR violation. I believe the discussion here gives more insights of how this works: https://reviews.llvm.org/D29877

Indeed the warning is noisy but it will potentially fix broken code which were unable to diagnose before. Especially that in some cases where static templates in header files are used as an idiom. In theory this approach can extend to cases described in https://wg21.link/p2691 where our inability to diagnose/fix these cases leads to some design decisions which may not be optimal.

[v.g.vassilev]

clang/test/SemaCXX/warn-func-not-needed.cpp

13

Nobody uses foo.

[philnik]

In D143524#4148024, @v.g.vassilev wrote:

In D143524#4148006, @philnik wrote:

The emitted warnings from the libc++ CI look like a false-positive to me. While the functions are never called, they are used in an unevaluated context. I would expect -Wunused warnings to only be emitted when I can just remove the code without problems, which doesn't seem to be the case here. It would probably just get turned off again by lots of people if there are too many false-positives, which I don't think is the goal here.

From what I see is that most of the templates are marked with static which means internal linkage. Entities with internal linkage in header files are essentially different across translation units which is an ODR violation. I believe the discussion here gives more insights of how this works: https://reviews.llvm.org/D29877

Indeed the warning is noisy but it will potentially fix broken code which were unable to diagnose before. Especially that in some cases where static templates in header files are used as an idiom. In theory this approach can extend to cases described in https://wg21.link/p2691 where our inability to diagnose/fix these cases leads to some design decisions which may not be optimal.

I missed the static at the beginning. That explains the warning, thanks! I agree this should be fixed. I'll look into making a patch to enable -Wunused-template and fix any problems. Hopefully there aren't too many.

[v.g.vassilev]

In D143524#4148039, @philnik wrote:

In D143524#4148024, @v.g.vassilev wrote:

In D143524#4148006, @philnik wrote:

The emitted warnings from the libc++ CI look like a false-positive to me. While the functions are never called, they are used in an unevaluated context. I would expect -Wunused warnings to only be emitted when I can just remove the code without problems, which doesn't seem to be the case here. It would probably just get turned off again by lots of people if there are too many false-positives, which I don't think is the goal here.

From what I see is that most of the templates are marked with static which means internal linkage. Entities with internal linkage in header files are essentially different across translation units which is an ODR violation. I believe the discussion here gives more insights of how this works: https://reviews.llvm.org/D29877

Indeed the warning is noisy but it will potentially fix broken code which were unable to diagnose before. Especially that in some cases where static templates in header files are used as an idiom. In theory this approach can extend to cases described in https://wg21.link/p2691 where our inability to diagnose/fix these cases leads to some design decisions which may not be optimal.

I missed the static at the beginning. That explains the warning, thanks! I agree this should be fixed. I'll look into making a patch to enable -Wunused-template and fix any problems. Hopefully there aren't too many.

This would be awesome as I don't have a lot of bandwidth right now and that potentially will have quite positive impact (once people understand what they need to do). I am not sure if we could somehow emit a fix-it suggestion. So far I have failed in figuring out what's a good suggestion when using this static template idiom...

[philnik]

It looks like this warning is incompatible with -Wctad-maybe-unsupported. It warns that the deduction guide is unused, but the deduction guide is required suppress -Wctad-maybe-unsupported. https://godbolt.org/z/G8bMjYsbn

[v.g.vassilev]

In D143524#4148271, @philnik wrote:

It looks like this warning is incompatible with -Wctad-maybe-unsupported. It warns that the deduction guide is unused, but the deduction guide is required suppress -Wctad-maybe-unsupported. https://godbolt.org/z/G8bMjYsbn

That should not be too hard to fix. It seems that just need to ignore CXXDeductionGuideDecl in Sema::ShouldRemoveFromUnused. Is that blocking you?

[philnik]

In D143524#4148344, @v.g.vassilev wrote:

In D143524#4148271, @philnik wrote:

It looks like this warning is incompatible with -Wctad-maybe-unsupported. It warns that the deduction guide is unused, but the deduction guide is required suppress -Wctad-maybe-unsupported. https://godbolt.org/z/G8bMjYsbn

That should not be too hard to fix. It seems that just need to ignore CXXDeductionGuideDecl in Sema::ShouldRemoveFromUnused. Is that blocking you?

No, I just added a [[maybe_unused]]. D144667 should fix any libc++ problems.

[aaron.ballman]

In D143524#4148024, @v.g.vassilev wrote:

Indeed the warning is noisy but it will potentially fix broken code which were unable to diagnose before. Especially that in some cases where static templates in header files are used as an idiom. In theory this approach can extend to cases described in https://wg21.link/p2691 where our inability to diagnose/fix these cases leads to some design decisions which may not be optimal.

We need to ensure the diagnostic is not so noisy that people disable it, though. That means a low false positive rate and a straight-forward way to silence the diagnostic on a case-by-case basis.

clang/test/SemaCXX/warn-func-not-needed.cpp

13

Ah, good point on foo not being used, but the question still stands -- how does the user silence this diagnostic? It's not at all uncommon to have a primary template with specializations where the TU only uses either the primary or a specialization, but not both (and certainly not all specializations).

[v.g.vassilev]

In D143524#4150286, @aaron.ballman wrote:

In D143524#4148024, @v.g.vassilev wrote:

Indeed the warning is noisy but it will potentially fix broken code which were unable to diagnose before. Especially that in some cases where static templates in header files are used as an idiom. In theory this approach can extend to cases described in https://wg21.link/p2691 where our inability to diagnose/fix these cases leads to some design decisions which may not be optimal.

We need to ensure the diagnostic is not so noisy that people disable it, though. That means a low false positive rate and a straight-forward way to silence the diagnostic on a case-by-case basis.

I agree, so far I have not seen false positives. All issues (except for -Wctad-maybe-unsupported as mentioned above) seemed real. Do you have a particular example in mind?

clang/test/SemaCXX/warn-func-not-needed.cpp

13

@philnik used [[maybe_unused]] which seemed reasonable to me for silencing the diagnostic. Maybe take a look at the changes done here: https://reviews.llvm.org/D144667

[aaron.ballman]

In D143524#4150289, @v.g.vassilev wrote:

In D143524#4150286, @aaron.ballman wrote:

In D143524#4148024, @v.g.vassilev wrote:

Indeed the warning is noisy but it will potentially fix broken code which were unable to diagnose before. Especially that in some cases where static templates in header files are used as an idiom. In theory this approach can extend to cases described in https://wg21.link/p2691 where our inability to diagnose/fix these cases leads to some design decisions which may not be optimal.

We need to ensure the diagnostic is not so noisy that people disable it, though. That means a low false positive rate and a straight-forward way to silence the diagnostic on a case-by-case basis.

I agree, so far I have not seen false positives. All issues (except for -Wctad-maybe-unsupported as mentioned above) seemed real. Do you have a particular example in mind?

The concrete example I have is the one under discussion from the test cases, but I was also speaking about the libc++ work that's going on as well. In general, it's best to try the diagnostic on some large-scale C++ projects to see what kind of results you get (compiling projects like LLVM, Qt, Kokkos, Chromium, etc). That usually helps spot other situations like -Wctad-maybe-unused etc.

clang/test/SemaCXX/warn-func-not-needed.cpp

13

That's reasonable if the interface is one the user controls, such as one within a .cpp file. But the situation I'm worried about is where the primary template and specializations live in a header file that's shared between multiple TUs. I don't think it's reasonable to expect users to put [[maybe_unused]] on the primary template and all specializations in that situation.

[ldionne]

clang/test/SemaCXX/warn-func-not-needed.cpp

13

Don't y'all find it weird to have to use [[maybe_unused]] on something that is only a declaration like those CTAD guides? And I agree with @aaron.ballman here: we provide headers that are used in various TUs, and we obviously never expect that the entirety of our headers is going to be used by every single TU.

In other words, we totally expect that those deduction guides will be unused in some cases, since it's entirely fine for a user not to use them but for us to still provide them. If I understood this correctly, this seems like a flaw in the warning that we should fix in Clang.

[v.g.vassilev]

clang/test/SemaCXX/warn-func-not-needed.cpp

13

Yes, I agree I am pretty sure we can fix the CTAD guides. I just need a few spare cycles...

[ldionne]

[Github PR transition cleanup]

Dropping libc++ since this shouldn't affect libc++ since D144667.

clang/include/clang/Basic/DiagnosticGroups.td

def Conversion : DiagGroup<"conversion",
                             ObjCLiteralConversion,
                             SignConversion,
                             StringConversion]>,
                  DiagCategory<"Value Conversion Issue">;
 
 def Unused : DiagGroup<"unused",
                        [UnusedArgument, UnusedFunction, UnusedLabel,
                         // UnusedParameter, (matches GCC's behavior)
-                        // UnusedTemplate, (clean-up libc++ before enabling)
+                        UnusedTemplate,
                         // UnusedMemberFunction, (clean-up llvm before enabling)
                         UnusedPrivateField, UnusedLambdaCapture,
                         UnusedLocalTypedef, UnusedValue, UnusedVariable,
                         UnusedButSetVariable, UnusedPropertyIvar]>,
                         DiagCategory<"Unused Entity Issue">;
 
 // Format settings.
 def FormatInvalidSpecifier : DiagGroup<"format-invalid-specifier">;

clang/test/Misc/warning-wall.c

 CHECK-NEXT:      -Wstatic-self-init
 CHECK-NEXT:      -Wuninitialized-const-reference
 CHECK-NEXT:    -Wunknown-pragmas
 CHECK-NEXT:    -Wunused
 CHECK-NEXT:      -Wunused-argument
 CHECK-NEXT:      -Wunused-function
 CHECK-NEXT:        -Wunneeded-internal-declaration
 CHECK-NEXT:      -Wunused-label
+CHECK-NEXT:      -Wunused-template
+CHECK-NEXT:        -Wunneeded-internal-declaration
 CHECK-NEXT:      -Wunused-private-field
 CHECK-NEXT:      -Wunused-lambda-capture
 CHECK-NEXT:      -Wunused-local-typedef
 CHECK-NEXT:      -Wunused-value
 CHECK-NEXT:        -Wunused-comparison
 CHECK-NEXT:        -Wunused-result
 CHECK-NEXT:        -Wunevaluated-expression
 CHECK-NEXT:          -Wpotentially-evaluated-expression

clang/test/SemaCXX/warn-func-not-needed.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wall %s
 
 namespace test1 {
 static void f() {} // expected-warning {{function 'f' is not needed and will not be emitted}}
 static void f();
 template <typename T>
 void foo() {
   f();
 }
 }
 
 namespace test1_template {
-template <typename T> static void f() {}
+template <typename T> static void f() {} // expected-warning {{unused function template}}
 template <> void f<int>() {} // expected-warning {{function 'f<int>' is not needed and will not be emitted}}
 template <typename T>
 void foo() {
   f<int>();
   f<long>();
 }
 } // namespace test1_template
 

clang/test/SemaCXX/warn-variable-not-needed.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wall %s
 
 namespace test1 {
   static int abc = 42; // expected-warning {{variable 'abc' is not needed and will not be emitted}}
 
   namespace {
-  template <typename T> int abc_template = 0;
+  template <typename T> int abc_template = 0; //expected-warning {{unused variable template}}
   template <> int abc_template<int> = 0; // expected-warning {{variable 'abc_template<int>' is not needed and will not be emitted}}
   }                                      // namespace
   template <typename T>
   int foo(void) {
     return abc + abc_template<int> + abc_template<long>;
   }
 }