aaron.ballman added inline comments.
================ Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:131 *Result.Context); - if (!IsConstQualified) + if (!CurrentParam.getType().getCanonicalType().isConstQualified()) Diag << utils::fixit::changeVarDeclToConst(CurrentParam); ---------------- You should add a comment here explaining why you need something more complex than `IsConstQualified`. ================ Comment at: test/clang-tidy/performance-unnecessary-value-param.cpp:242 +// 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); ---------------- flx wrote: > aaron.ballman wrote: > > 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? > Good catch. I added the reverse case as well and modified the check slightly > to make that case work as well. 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. Repository: rL LLVM https://reviews.llvm.org/D26207 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits