[clang-tidy] Do not trigger unnecessary-value-param check on methods marked as final

[flx]

Summary

Virtual method overrides of dependent types cannot be recognized unless they are marked as override or final.

Exclude methods marked as final from check and add test.

[hokein]

LGTM.

[malcolm.parsons]

test/clang-tidy/performance-unnecessary-value-param.cpp

276 ↗

(On Diff #79701)

Do we want to warn about methods in templated classes at all?

284 ↗

(On Diff #79701)

Typo Negativ -> Negative.

[flx]

test/clang-tidy/performance-unnecessary-value-param.cpp

276 ↗

(On Diff #79701)

I think we should still trigger on methods where we can determine that the parameter type is expensive to copy. Note that this is not the case when the type is dependent.

[malcolm.parsons]

test/clang-tidy/performance-unnecessary-value-param.cpp

276 ↗

(On Diff #79701)

We still have issues when the overriding method is not marked override or final.
Is there another way to handle this?

[alexfh]

LG. Looks like a strict improvement.

[flx]

test/clang-tidy/performance-unnecessary-value-param.cpp

276 ↗

(On Diff #79701)

I filed https://llvm.org/bugs/show_bug.cgi?id=31236 to keep track of fixing the check for this case as well.

[malcolm.parsons]

test/clang-tidy/performance-unnecessary-value-param.cpp

276 ↗

(On Diff #79701)

Thanks.

clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

: ClangTidyCheck(Name, Context),
           Options.get("IncludeStyle", "llvm"))) {}
 
 void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
   const auto ExpensiveValueParamDecl =
       parmVarDecl(hasType(hasCanonicalType(allOf(matchers::isExpensiveToCopy(),
                                                  unless(referenceType())))),
                   decl().bind("param"));
   Finder->addMatcher(
-      functionDecl(isDefinition(), unless(cxxMethodDecl(isOverride())),
+      functionDecl(isDefinition(),
+                   unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
                    unless(isInstantiated()),
                    has(typeLoc(forEach(ExpensiveValueParamDecl))),
                    decl().bind("functionDecl")),
       this);
 }
 
 void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");

clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp

 void PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType A) {
   // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'A' is copied
   // CHECK-FIXES: void PositiveMessageAndFixAsFunctionIsCalled(const ExpensiveToCopyType& A) {
 }
 
 void ReferenceFunctionByCallingIt() {
   PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
 }
+
+// Virtual method overrides of dependent types cannot be recognized unless they
+// are marked as override or final. Test that check is not triggered on methods
+// marked with override or final.
+template <typename T>
+struct NegativeDependentTypeInterface {
+  virtual void Method(ExpensiveToCopyType E) = 0;
+};
+
+template <typename T>
+struct NegativeOverrideImpl : public NegativeDependentTypeInterface<T> {
+  void Method(ExpensiveToCopyType E) override {}
+};
+
+template <typename T>
+struct NegativeFinalImpl : public NegativeDependentTypeInterface<T> {
+  void Method(ExpensiveToCopyType E) final {}
+};