This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by flx on Nov 1 2016, 11:20 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

flx updated this revision to Diff 76606.Nov 1 2016, 11:20 AM
flx retitled this revision from to [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified.
flx updated this object.
flx added reviewers: alexfh, sbenza, aaron.ballman.
flx set the repository for this revision to rL LLVM.
flx added a subscriber: cfe-commits.
aaron.ballman added inline comments.Nov 1 2016, 12:10 PM
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 updated this revision to Diff 76630.Nov 1 2016, 2:18 PM
flx added inline comments.
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 added inline comments.Nov 2 2016, 9:42 AM
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 added inline comments.Nov 2 2016, 2:59 PM
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 added inline comments.Nov 3 2016, 6:43 AM
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 updated this revision to Diff 76877.Nov 3 2016, 12:11 PM
flx removed rL LLVM as the repository for this revision.
flx marked an inline comment as done.
flx added inline comments.
test/clang-tidy/performance-unnecessary-value-param.cpp
242
flx added a comment.Nov 4 2016, 8:40 AM

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

aaron.ballman accepted this revision.Nov 4 2016, 12:27 PM
aaron.ballman edited edge metadata.

LGTM!

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

Thank you for filing the bug report.

This revision is now accepted and ready to land.Nov 4 2016, 12:27 PM
This revision was automatically updated to reflect the committed changes.