aaron.ballman added inline comments.

================
Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:131
                                                            *Result.Context);
-    if (!IsConstQualified)
+    if (!CurrentParam.getType().getCanonicalType().isConstQualified())
       Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
----------------
You should add a comment here explaining why you need something more complex 
than `IsConstQualified`.


================
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:
> > 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.


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

Reply via email to