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