flx added inline comments.
================
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);
----------------
aaron.ballman wrote:
> 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.
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?
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
