aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
================
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:
> > > > 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 (
Thank you for filing the bug report.
https://reviews.llvm.org/D26207
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
