[ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified

[aaron.ballman]

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

242

This comment doesn't really match the test cases. The original code has two *different* declarations (only one of which is a definition), not one declaration and one redeclaration with the definition.

I think what is really happening is that it is adding the & qualifier to the first declaration, and adding the const and & qualifiers to the second declaration, and the result is that you get harmonization. But it brings up a question to me; what happens with:

void f1(ExpensiveToCopyType A) {
}

void f1(const ExpensiveToCopyType A) {

}

Does the fix-it try to create two definitions of the same function?

244

Is the CHECK-MESSAGES missing?

[flx]

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

242

Good catch. I added the reverse case as well and modified the check slightly to make that case work as well.

244

The message is only generated on the definition below, the declaration will only have a fix.

[aaron.ballman]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

131–132

You should add a comment here explaining why you need something more complex than IsConstQualified.

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

242

Can you add a test like this as well?

void f1(ExpensiveToCopyType A); // Declared, not defined

void f1(const ExpensiveToCopyType A) {}
void f1(const ExpensiveToCopyType &A) {}

I'm trying to make sure this check does not suggest a fixit that breaks existing code because of overload sets. I would expect a diagnostic for the first two declarations, but no fixit suggestion for void f1(const ExpensiveToCopyType A) because that would result in an ambiguous function definition.

[flx]

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

242

The current code suggests the following fixes:

-void f1(ExpensiveToCopyType A); Declared, not defined
+void f1(const ExpensiveToCopyType& A); Declared, not defined

-void f1(const ExpensiveToCopyType A) {}
+void f1(const ExpensiveToCopyType& A) {}
void f1(const ExpensiveToCopyType &A) {}

and we get a warning message for the void f1(const ExpensiveToCopyType A) {}

I think the compiler sees "void f1(const ExpensiveToCopyType A) {}" as definition of "void f1(ExpensiveToCopyType A); // Declared, not defined" and thus both places get fixed.

Since the check is catching cases where the value argument is either modified or could be moved it and this is not the case here it is worth raising this issue of what looks like a very subtle difference in overrides.

My inclination is to leave this as is. What do you suggest we do?

[aaron.ballman]

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

242

I think this behavior isn't new with your changes, so we can leave it as is. However, we should file a bug report about the bad behavior with overload sets (and provide the example that's failing) so that we don't forget to come back and fix the functionality.

[flx]

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

242

Filed to track this: https://llvm.org/bugs/show_bug.cgi?id=30902 (

[flx]

Aaron, do you have any other comments or does the patch look good to you?

[aaron.ballman]

LGTM!

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

242

Thank you for filing the bug report.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Method = llvm::dyn_cast<CXXMethodDecl>(Function);
   if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()))
     return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
        FunctionDecl = FunctionDecl->getPreviousDecl()) {
     const auto &CurrentParam = *FunctionDecl->getParamDecl(Index);
     Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
                                                            *Result.Context);
-    if (!IsConstQualified)
+    if (!IsConstQualified &&
+        !CurrentParam.getType().getCanonicalType().isConstQualified())
       Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
   }
 }
 
 void UnnecessaryValueParamCheck::registerPPCallbacks(
     CompilerInstance &Compiler) {
   Inserter.reset(new utils::IncludeInserter(
       Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));

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

 }
 
 void PositiveConstRefNotMoveAssignable(ExpensiveToCopyType A) {
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the parameter 'A' is copied
   // CHECK-FIXES: void PositiveConstRefNotMoveAssignable(const ExpensiveToCopyType& A) {
   ExpensiveToCopyType B;
   B = A;
 }
+
+// Case where parameter in declaration is already const-qualified but not in
+// implementation. Make sure a second 'const' is not added to the declaration.
+void PositiveConstDeclaration(const ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}