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

[Felix Berger via cfe-commits]

flx marked an inline comment as done.
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:
> > > 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.
Filed to track this: https://llvm.org/bugs/show_bug.cgi?id=30902 (


https://reviews.llvm.org/D26207