[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
This revision was automatically updated to reflect the committed changes.
Closed by commit rL286010: [ClangTidy - performance-unnecessary-value-param]
Only add "const" when current… (authored by flx).
Changed prior to commit:
https://reviews.llvm.org/D26207?vs=76877=76940#toc
Repository:
rL LLVM
https://reviews.llvm.org/D26207
Files:
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
Index:
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
---
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -128,7 +128,10 @@
const auto = *FunctionDecl->getParamDecl(Index);
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
-if (!IsConstQualified)
+// The parameter of each declaration needs to be checked individually as to
+// whether it is const or not as constness can differ between definition
and
+// declaration.
+if (!CurrentParam.getType().getCanonicalType().isConstQualified())
Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
}
}
Index:
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
===
---
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
+++
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -244,3 +244,19 @@
void NegativeForIncompleteType(IncompleteType I) {
// CHECK-MESSAGES: [[@LINE-1]]:47: error: variable has incomplete type
'IncompleteType' [clang-diagnostic-error]
}
+
+// 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);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}
+
+void PositiveNonConstDeclaration(ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
+ // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType&
A) {
+}
Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -128,7 +128,10 @@
const auto = *FunctionDecl->getParamDecl(Index);
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
-if (!IsConstQualified)
+// The parameter of each declaration needs to be checked individually as to
+// whether it is const or not as constness can differ between definition and
+// declaration.
+if (!CurrentParam.getType().getCanonicalType().isConstQualified())
Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
}
}
Index: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -244,3 +244,19 @@
void NegativeForIncompleteType(IncompleteType I) {
// CHECK-MESSAGES: [[@LINE-1]]:47: error: variable has incomplete type 'IncompleteType' [clang-diagnostic-error]
}
+
+// 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);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}
+
+void PositiveNonConstDeclaration(ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
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 ) {}
> > > > ```
> > > > 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 ) {}
> > >
> > > 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
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
flx added a comment. Aaron, do you have any other comments or does the patch look good to you? https://reviews.llvm.org/D26207 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
flx removed rL LLVM as the repository for this revision.
flx updated this revision to Diff 76877.
https://reviews.llvm.org/D26207
Files:
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -237,3 +237,19 @@
ExpensiveToCopyType B;
B = A;
}
+
+// 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);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}
+
+void PositiveNonConstDeclaration(ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
+ // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType&
A) {
+}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -128,7 +128,10 @@
const auto = *FunctionDecl->getParamDecl(Index);
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
-if (!IsConstQualified)
+// The parameter of each declaration needs to be checked individually as to
+// whether it is const or not as constness can differ between definition
and
+// declaration.
+if (!CurrentParam.getType().getCanonicalType().isConstQualified())
Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
}
}
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -237,3 +237,19 @@
ExpensiveToCopyType B;
B = A;
}
+
+// 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);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}
+
+void PositiveNonConstDeclaration(ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
+ // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A) {
+}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -128,7 +128,10 @@
const auto = *FunctionDecl->getParamDecl(Index);
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
-if (!IsConstQualified)
+// The parameter of each declaration needs to be checked individually as to
+// whether it is const or not as constness can differ between definition and
+// declaration.
+if (!CurrentParam.getType().getCanonicalType().isConstQualified())
Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
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 ) {}
> > > ```
> > > 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 ) {}
> >
> > 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
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
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 ) {}
> > ```
> > 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 ) {}
>
> 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
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
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:
> > > 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 ) {}
> ```
> 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 ) {}
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?
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
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
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 ) {}
```
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
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
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:
> 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.
Comment at: test/clang-tidy/performance-unnecessary-value-param.cpp:244
+void PositiveConstDeclaration(const ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
aaron.ballman wrote:
> Is the `CHECK-MESSAGES` missing?
The message is only generated on the definition below, the declaration will
only have a fix.
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
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
flx updated this revision to Diff 76630.
Repository:
rL LLVM
https://reviews.llvm.org/D26207
Files:
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -237,3 +237,19 @@
ExpensiveToCopyType B;
B = A;
}
+
+// 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);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}
+
+void PositiveNonConstDeclaration(ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
+ // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType&
A) {
+}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -128,7 +128,7 @@
const auto = *FunctionDecl->getParamDecl(Index);
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
-if (!IsConstQualified)
+if (!CurrentParam.getType().getCanonicalType().isConstQualified())
Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
}
}
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -237,3 +237,19 @@
ExpensiveToCopyType B;
B = A;
}
+
+// 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);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}
+
+void PositiveNonConstDeclaration(ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
+ // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A) {
+}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -128,7 +128,7 @@
const auto = *FunctionDecl->getParamDecl(Index);
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
-if (!IsConstQualified)
+if (!CurrentParam.getType().getCanonicalType().isConstQualified())
Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
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);
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?
Comment at: test/clang-tidy/performance-unnecessary-value-param.cpp:244
+void PositiveConstDeclaration(const ExpensiveToCopyType A);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
Is the `CHECK-MESSAGES` missing?
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
[PATCH] D26207: [ClangTidy - performance-unnecessary-value-param] Only add "const" when current parameter is not already const qualified
flx created this revision.
flx added reviewers: alexfh, sbenza, aaron.ballman.
flx added a subscriber: cfe-commits.
flx set the repository for this revision to rL LLVM.
Repository:
rL LLVM
https://reviews.llvm.org/D26207
Files:
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -237,3 +237,12 @@
ExpensiveToCopyType B;
B = A;
}
+
+// 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);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -128,7 +128,8 @@
const auto = *FunctionDecl->getParamDecl(Index);
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
-if (!IsConstQualified)
+if (!IsConstQualified &&
+!CurrentParam.getType().getCanonicalType().isConstQualified())
Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
}
}
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -237,3 +237,12 @@
ExpensiveToCopyType B;
B = A;
}
+
+// 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);
+// CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A);
+void PositiveConstDeclaration(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:51: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
+}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -128,7 +128,8 @@
const auto = *FunctionDecl->getParamDecl(Index);
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
*Result.Context);
-if (!IsConstQualified)
+if (!IsConstQualified &&
+!CurrentParam.getType().getCanonicalType().isConstQualified())
Diag << utils::fixit::changeVarDeclToConst(CurrentParam);
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
