Page MenuHomePhabricator

Ignore incomplete types when determining whether they are expensive to copy
ClosedPublic

Authored by flx on Nov 1 2016, 8:44 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

flx updated this revision to Diff 76563.Nov 1 2016, 8:44 AM
flx retitled this revision from to Ignore incomplete types when determining whether they are expensive to copy.
flx updated this object.
flx added a reviewer: alexfh.
flx set the repository for this revision to rL LLVM.
flx added a subscriber: cfe-commits.

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

flx added a comment.Nov 1 2016, 9:32 AM

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.

In D26195#584724, @flx 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 added a comment.Nov 1 2016, 11:09 AM
In D26195#584724, @flx 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?

In D26195#584958, @flx wrote:
In D26195#584724, @flx 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 added a comment.Nov 1 2016, 2:21 PM
In D26195#584958, @flx wrote:
In D26195#584724, @flx 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?

In D26195#585198, @flx wrote:
In D26195#584958, @flx wrote:
In D26195#584724, @flx 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 updated this revision to Diff 76928.Nov 4 2016, 12:10 PM
flx removed rL LLVM as the repository for this revision.
flx added a reviewer: aaron.ballman.
In D26195#585198, @flx wrote:
In D26195#584958, @flx wrote:
In D26195#584724, @flx 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 accepted this revision.Nov 4 2016, 12:31 PM
aaron.ballman edited edge metadata.

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

This revision is now accepted and ready to land.Nov 4 2016, 12:31 PM
flx updated this revision to Diff 76934.Nov 4 2016, 1:23 PM
flx edited edge metadata.
flx marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
alexfh edited edge metadata.Nov 4 2016, 9:41 PM

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 added a comment.Nov 7 2016, 1:20 PM
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
241
alexfh added inline comments.Nov 7 2016, 3:51 PM
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
241

Thank you!