Ignore incomplete types when determining whether they are expensive to copy

[flx]

Summary

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.

[aaron.ballman]

Please add a test case with an incomplete type that would exercise this code path, otherwise, LGTM.

[flx]

In 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.

[aaron.ballman]

In D26195#584724, @flx wrote:

In 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().

[flx]

In D26195#584730, @aaron.ballman wrote:

In D26195#584724, @flx wrote:

In 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?

[aaron.ballman]

In D26195#584958, @flx wrote:

In D26195#584730, @aaron.ballman wrote:

In D26195#584724, @flx wrote:

In 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."

[flx]

In D26195#585091, @aaron.ballman wrote:

In D26195#584958, @flx wrote:

In D26195#584730, @aaron.ballman wrote:

In D26195#584724, @flx wrote:

In 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?

[aaron.ballman]

In D26195#585198, @flx wrote:

In D26195#585091, @aaron.ballman wrote:

In D26195#584958, @flx wrote:

In D26195#584730, @aaron.ballman wrote:

In D26195#584724, @flx wrote:

In 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?

[flx]

In D26195#585917, @aaron.ballman wrote:

In D26195#585198, @flx wrote:

In D26195#585091, @aaron.ballman wrote:

In D26195#584958, @flx wrote:

In D26195#584730, @aaron.ballman wrote:

In D26195#584724, @flx wrote:

In 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.

[aaron.ballman]

LGTM, modulo a small commenting request.

test/clang-tidy/performance-unnecessary-value-param.cpp

242 ↗

(On Diff #76928)

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."

[alexfh]

Thank you for the fix! One late comment inline.

clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp

241

Please move this test to a separate test file and revert the RUN line here.

[flx]

clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp

241

Done in https://reviews.llvm.org/D26369.

[alexfh]

clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp

241

Thank you!

clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp

bool hasDeletedCopyConstructor(QualType Type) {
   }
   return false;
 }
 
 } // namespace
 
 llvm::Optional<bool> isExpensiveToCopy(QualType Type,
                                        const ASTContext &Context) {
-  if (Type->isDependentType())
+  if (Type->isDependentType() || Type->isIncompleteType())
     return llvm::None;
   return !Type.isTriviallyCopyableType(Context) &&
          !classHasTrivialCopyAndDestroy(Type) &&
          !hasDeletedCopyConstructor(Type);
 }
 
 bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
                                            const ASTContext &Context) {

clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp

-// 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 <utility>
 
 struct ExpensiveToCopyType {
   const ExpensiveToCopyType & constReference() const {
     return *this;
   }
   void nonConstMethod();

 }
 
 void PositiveConstRefNotMoveAssignable(ExpensiveToCopyType A) {
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the parameter 'A' is copied
   // CHECK-FIXES: void PositiveConstRefNotMoveAssignable(const ExpensiveToCopyType& A) {
   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]
+}