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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
