[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
alexfh added inline comments. Comment at: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp:241 + +// Ensure that incomplete types result in an error from the frontend and not a +// clang-tidy diagnostic about IncompleteType being expensive to copy. flx wrote: > alexfh wrote: > > Please move this test to a separate test file and revert the RUN line here. > Done in https://reviews.llvm.org/D26369. Thank you! Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
flx added a comment. Comment at: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp:241 + +// Ensure that incomplete types result in an error from the frontend and not a +// clang-tidy diagnostic about IncompleteType being expensive to copy. alexfh wrote: > Please move this test to a separate test file and revert the RUN line here. Done in https://reviews.llvm.org/D26369. Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
alexfh added a comment. Thank you for the fix! One late comment inline. Comment at: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp:241 + +// Ensure that incomplete types result in an error from the frontend and not a +// clang-tidy diagnostic about IncompleteType being expensive to copy. Please move this test to a separate test file and revert the RUN line here. Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
This revision was automatically updated to reflect the committed changes.
Closed by commit rL286008: [clang-tidy] Ignore incomplete types when
determining whether they are… (authored by flx).
Changed prior to commit:
https://reviews.llvm.org/D26195?vs=76934=76935#toc
Repository:
rL LLVM
https://reviews.llvm.org/D26195
Files:
clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
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
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t --
-fix-errors -- --std=c++11
// CHECK-FIXES: #include
@@ -237,3 +237,10 @@
ExpensiveToCopyType B;
B = A;
}
+
+// Ensure that incomplete types result in an error from the frontend and not a
+// clang-tidy diagnostic about IncompleteType being expensive to copy.
+struct IncompleteType;
+void NegativeForIncompleteType(IncompleteType I) {
+ // CHECK-MESSAGES: [[@LINE-1]]:47: error: variable has incomplete type
'IncompleteType' [clang-diagnostic-error]
+}
Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
@@ -41,7 +41,7 @@
llvm::Optional isExpensiveToCopy(QualType Type,
const ASTContext ) {
- if (Type->isDependentType())
+ if (Type->isDependentType() || Type->isIncompleteType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
!classHasTrivialCopyAndDestroy(Type) &&
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
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -fix-errors -- --std=c++11
// CHECK-FIXES: #include
@@ -237,3 +237,10 @@
ExpensiveToCopyType B;
B = A;
}
+
+// Ensure that incomplete types result in an error from the frontend and not a
+// clang-tidy diagnostic about IncompleteType being expensive to copy.
+struct IncompleteType;
+void NegativeForIncompleteType(IncompleteType I) {
+ // CHECK-MESSAGES: [[@LINE-1]]:47: error: variable has incomplete type 'IncompleteType' [clang-diagnostic-error]
+}
Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
@@ -41,7 +41,7 @@
llvm::Optional isExpensiveToCopy(QualType Type,
const ASTContext ) {
- if (Type->isDependentType())
+ if (Type->isDependentType() || Type->isIncompleteType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
!classHasTrivialCopyAndDestroy(Type) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, modulo a small commenting request.
Comment at: test/clang-tidy/performance-unnecessary-value-param.cpp:242
+struct IncompleteType;
+void NegativeForIncompleteType(IncompleteType I) {
+ // CHECK-MESSAGES: [[@LINE-1]]:47: error: variable has incomplete type
'IncompleteType' [clang-diagnostic-error]
You should put a comment before this function that explains why this
seemingly-incongruous test lives here. Something along the lines of "Ensure
that incomplete types result in an error from the frontend and not a clang-tidy
diagnostic about IncompleteType being expensive to copy."
https://reviews.llvm.org/D26195
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
flx added a comment. In https://reviews.llvm.org/D26195#585917, @aaron.ballman wrote: > In https://reviews.llvm.org/D26195#585198, @flx wrote: > > > In https://reviews.llvm.org/D26195#585091, @aaron.ballman wrote: > > > > > In https://reviews.llvm.org/D26195#584958, @flx wrote: > > > > > > > In https://reviews.llvm.org/D26195#584730, @aaron.ballman wrote: > > > > > > > > > In https://reviews.llvm.org/D26195#584724, @flx wrote: > > > > > > > > > > > In https://reviews.llvm.org/D26195#584712, @aaron.ballman wrote: > > > > > > > > > > > > > Please add a test case with an incomplete type that would > > > > > > > exercise this code path, otherwise, LGTM. > > > > > > > > > > > > > > > > > > Hi Aaron, > > > > > > > > > > > > do you have any advise on how to add an incomplete type? When > > > > > > debugging this I had a compilation unit that failed to compile > > > > > > causing it, but I'm not sure this is a good way to add a test case. > > > > > > > > > > > > > > > A type like `class C;` is an incomplete type, as is `void`, so > > > > > perhaps you can find a check that would let such a construct call > > > > > through to `isExpensiveToCopy()`. > > > > > > > > > > > > Great, this works and I was able to see the check produce a false > > > > positive without the proposed change here, but the test code introduces > > > > a compile error now due to the incomplete type used in the function > > > > definition. Is there a way to suppress that? > > > > > > > > > Unlikely -- fixing the compile error likely makes the type not expensive > > > to copy by using a pointer (or reference). This may be tricky to test > > > because the times when you would call `isExpensiveToCopy()` is with types > > > that are going to be logically required to be complete. I am not certain > > > the compile error is actually a problem though -- I would imagine your > > > existing false-positives (that you mentioned in the patch summary) are > > > cases where there is a compile error *and* a clang-tidy diagnostic, so > > > the test may simply be "check that there's only a compile error and no > > > clang-tidy diagnostic where there used to be a false-positive one." > > > > > > That's exactly the case, my question here is how can I make the test > > succeed in the face of a compile error, i.e. by expecting the error as well? > > > Doesn't `// CHECK-MESSAGES: :[[@LINE-1]]:3: error: blah blah blah` work? Yes, that was needed. I also had to change the run command to add "-fix-errors" to still apply fixes in the face of compile errors. I also added you as reviewer, Aaron. https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
flx removed rL LLVM as the repository for this revision.
flx updated this revision to Diff 76928.
https://reviews.llvm.org/D26195
Files:
clang-tidy/utils/TypeTraits.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
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t --
-fix-errors -- --std=c++11
// CHECK-FIXES: #include
@@ -237,3 +237,8 @@
ExpensiveToCopyType B;
B = A;
}
+
+struct IncompleteType;
+void NegativeForIncompleteType(IncompleteType I) {
+ // CHECK-MESSAGES: [[@LINE-1]]:47: error: variable has incomplete type
'IncompleteType' [clang-diagnostic-error]
+}
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -41,7 +41,7 @@
llvm::Optional isExpensiveToCopy(QualType Type,
const ASTContext ) {
- if (Type->isDependentType())
+ if (Type->isDependentType() || Type->isIncompleteType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
!classHasTrivialCopyAndDestroy(Type) &&
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
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -fix-errors -- --std=c++11
// CHECK-FIXES: #include
@@ -237,3 +237,8 @@
ExpensiveToCopyType B;
B = A;
}
+
+struct IncompleteType;
+void NegativeForIncompleteType(IncompleteType I) {
+ // CHECK-MESSAGES: [[@LINE-1]]:47: error: variable has incomplete type 'IncompleteType' [clang-diagnostic-error]
+}
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -41,7 +41,7 @@
llvm::Optional isExpensiveToCopy(QualType Type,
const ASTContext ) {
- if (Type->isDependentType())
+ if (Type->isDependentType() || Type->isIncompleteType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
!classHasTrivialCopyAndDestroy(Type) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
aaron.ballman added a comment. In https://reviews.llvm.org/D26195#585198, @flx wrote: > In https://reviews.llvm.org/D26195#585091, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D26195#584958, @flx wrote: > > > > > In https://reviews.llvm.org/D26195#584730, @aaron.ballman wrote: > > > > > > > In https://reviews.llvm.org/D26195#584724, @flx wrote: > > > > > > > > > In https://reviews.llvm.org/D26195#584712, @aaron.ballman wrote: > > > > > > > > > > > Please add a test case with an incomplete type that would exercise > > > > > > this code path, otherwise, LGTM. > > > > > > > > > > > > > > > Hi Aaron, > > > > > > > > > > do you have any advise on how to add an incomplete type? When > > > > > debugging this I had a compilation unit that failed to compile > > > > > causing it, but I'm not sure this is a good way to add a test case. > > > > > > > > > > > > A type like `class C;` is an incomplete type, as is `void`, so perhaps > > > > you can find a check that would let such a construct call through to > > > > `isExpensiveToCopy()`. > > > > > > > > > Great, this works and I was able to see the check produce a false > > > positive without the proposed change here, but the test code introduces a > > > compile error now due to the incomplete type used in the function > > > definition. Is there a way to suppress that? > > > > > > Unlikely -- fixing the compile error likely makes the type not expensive to > > copy by using a pointer (or reference). This may be tricky to test because > > the times when you would call `isExpensiveToCopy()` is with types that are > > going to be logically required to be complete. I am not certain the compile > > error is actually a problem though -- I would imagine your existing > > false-positives (that you mentioned in the patch summary) are cases where > > there is a compile error *and* a clang-tidy diagnostic, so the test may > > simply be "check that there's only a compile error and no clang-tidy > > diagnostic where there used to be a false-positive one." > > > That's exactly the case, my question here is how can I make the test succeed > in the face of a compile error, i.e. by expecting the error as well? Doesn't `// CHECK-MESSAGES: :[[@LINE-1]]:3: error: blah blah blah` work? Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
flx added a comment. In https://reviews.llvm.org/D26195#585091, @aaron.ballman wrote: > In https://reviews.llvm.org/D26195#584958, @flx wrote: > > > In https://reviews.llvm.org/D26195#584730, @aaron.ballman wrote: > > > > > In https://reviews.llvm.org/D26195#584724, @flx wrote: > > > > > > > In https://reviews.llvm.org/D26195#584712, @aaron.ballman wrote: > > > > > > > > > Please add a test case with an incomplete type that would exercise > > > > > this code path, otherwise, LGTM. > > > > > > > > > > > > Hi Aaron, > > > > > > > > do you have any advise on how to add an incomplete type? When debugging > > > > this I had a compilation unit that failed to compile causing it, but > > > > I'm not sure this is a good way to add a test case. > > > > > > > > > A type like `class C;` is an incomplete type, as is `void`, so perhaps > > > you can find a check that would let such a construct call through to > > > `isExpensiveToCopy()`. > > > > > > Great, this works and I was able to see the check produce a false positive > > without the proposed change here, but the test code introduces a compile > > error now due to the incomplete type used in the function definition. Is > > there a way to suppress that? > > > Unlikely -- fixing the compile error likely makes the type not expensive to > copy by using a pointer (or reference). This may be tricky to test because > the times when you would call `isExpensiveToCopy()` is with types that are > going to be logically required to be complete. I am not certain the compile > error is actually a problem though -- I would imagine your existing > false-positives (that you mentioned in the patch summary) are cases where > there is a compile error *and* a clang-tidy diagnostic, so the test may > simply be "check that there's only a compile error and no clang-tidy > diagnostic where there used to be a false-positive one." That's exactly the case, my question here is how can I make the test succeed in the face of a compile error, i.e. by expecting the error as well? Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
aaron.ballman added a comment. In https://reviews.llvm.org/D26195#584958, @flx wrote: > In https://reviews.llvm.org/D26195#584730, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D26195#584724, @flx wrote: > > > > > In https://reviews.llvm.org/D26195#584712, @aaron.ballman wrote: > > > > > > > Please add a test case with an incomplete type that would exercise this > > > > code path, otherwise, LGTM. > > > > > > > > > Hi Aaron, > > > > > > do you have any advise on how to add an incomplete type? When debugging > > > this I had a compilation unit that failed to compile causing it, but I'm > > > not sure this is a good way to add a test case. > > > > > > A type like `class C;` is an incomplete type, as is `void`, so perhaps you > > can find a check that would let such a construct call through to > > `isExpensiveToCopy()`. > > > Great, this works and I was able to see the check produce a false positive > without the proposed change here, but the test code introduces a compile > error now due to the incomplete type used in the function definition. Is > there a way to suppress that? Unlikely -- fixing the compile error likely makes the type not expensive to copy by using a pointer (or reference). This may be tricky to test because the times when you would call `isExpensiveToCopy()` is with types that are going to be logically required to be complete. I am not certain the compile error is actually a problem though -- I would imagine your existing false-positives (that you mentioned in the patch summary) are cases where there is a compile error *and* a clang-tidy diagnostic, so the test may simply be "check that there's only a compile error and no clang-tidy diagnostic where there used to be a false-positive one." Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
flx added a comment. In https://reviews.llvm.org/D26195#584730, @aaron.ballman wrote: > In https://reviews.llvm.org/D26195#584724, @flx wrote: > > > In https://reviews.llvm.org/D26195#584712, @aaron.ballman wrote: > > > > > Please add a test case with an incomplete type that would exercise this > > > code path, otherwise, LGTM. > > > > > > Hi Aaron, > > > > do you have any advise on how to add an incomplete type? When debugging > > this I had a compilation unit that failed to compile causing it, but I'm > > not sure this is a good way to add a test case. > > > A type like `class C;` is an incomplete type, as is `void`, so perhaps you > can find a check that would let such a construct call through to > `isExpensiveToCopy()`. Great, this works and I was able to see the check produce a false positive without the proposed change here, but the test code introduces a compile error now due to the incomplete type used in the function definition. Is there a way to suppress that? Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
aaron.ballman added a comment. In https://reviews.llvm.org/D26195#584724, @flx wrote: > In https://reviews.llvm.org/D26195#584712, @aaron.ballman wrote: > > > Please add a test case with an incomplete type that would exercise this > > code path, otherwise, LGTM. > > > Hi Aaron, > > do you have any advise on how to add an incomplete type? When debugging this > I had a compilation unit that failed to compile causing it, but I'm not sure > this is a good way to add a test case. A type like `class C;` is an incomplete type, as is `void`, so perhaps you can find a check that would let such a construct call through to `isExpensiveToCopy()`. Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
flx added a comment. In https://reviews.llvm.org/D26195#584712, @aaron.ballman wrote: > Please add a test case with an incomplete type that would exercise this code > path, otherwise, LGTM. Hi Aaron, do you have any advise on how to add an incomplete type? When debugging this I had a compilation unit that failed to compile causing it, but I'm not sure this is a good way to add a test case. Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
aaron.ballman added a comment. Please add a test case with an incomplete type that would exercise this code path, otherwise, LGTM. Repository: rL LLVM https://reviews.llvm.org/D26195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26195: Ignore incomplete types when determining whether they are expensive to copy
flx created this revision.
flx added a reviewer: alexfh.
flx added a subscriber: cfe-commits.
flx set the repository for this revision to rL LLVM.
IsExpensiveToCopy can return false positives for incomplete types, so ignore
them.
All existing ClangTidy tests that depend on this function still pass as the
types are complete.
Repository:
rL LLVM
https://reviews.llvm.org/D26195
Files:
clang-tidy/utils/TypeTraits.cpp
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -41,7 +41,7 @@
llvm::Optional isExpensiveToCopy(QualType Type,
const ASTContext ) {
- if (Type->isDependentType())
+ if (Type->isDependentType() || Type->isIncompleteType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
!classHasTrivialCopyAndDestroy(Type) &&
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -41,7 +41,7 @@
llvm::Optional isExpensiveToCopy(QualType Type,
const ASTContext ) {
- if (Type->isDependentType())
+ if (Type->isDependentType() || Type->isIncompleteType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
!classHasTrivialCopyAndDestroy(Type) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
