This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] performance-unnecessary-copy-initialization: Prevent false positives when dependent variable is modified.
ClosedPublic

Authored by flx on Nov 20 2020, 1:38 PM.

Details

Summary

Extend the check to not only look at the variable the unnecessarily copied
variable is initialized from, but also ensure that any variable the old variable
references is not modified.

Extend DeclRefExprUtils to also count references and pointers to const assigned
from the DeclRef we check for const usages.

Diff Detail

Event Timeline

flx created this revision.Nov 20 2020, 1:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2020, 1:38 PM
flx requested review of this revision.Nov 20 2020, 1:38 PM
flx updated this revision to Diff 306773.Nov 20 2020, 1:44 PM

Fix formatting and comments.

Eugene.Zelenko added a project: Restricted Project.
flx updated this revision to Diff 309361.Dec 3 2020, 2:02 PM

Fixed clang tidy warnings.

flx added a comment.Dec 7 2020, 6:16 AM

Could someone take a first pass at this change? It would be great progress on this as this is the last currently known case that generates false positives.

I have some quick drive-by comments but still have to think about the test cases a bit more.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
73

typo: the the

76

typo: it its

82

You should mark the function as being static.

85

Elide braces around single-line if statements. For the cases involving comments, I'd recommend hoisting the comments above the if when returning a constant.

89

No need for the llvm::, is there? This should be replaced with !isa<ReferenceType, PointerType>(T) since you only care about the Boolean results.

flx updated this revision to Diff 310285.Dec 8 2020, 10:50 AM
flx marked 5 inline comments as done.

Address review comments.

flx added a comment.Dec 8 2020, 10:53 AM

Thanks for the review, Aaron! I appreciate it. Please let me know if you have any questions about the test cases.

aaron.ballman added inline comments.Dec 10 2020, 6:59 AM
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
91
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
492

Making sure I understand properly, if you were calling Orig.constMethod() then the diagnostic should be triggered, correct? Assuming so, having those test cases would be useful.

flx updated this revision to Diff 310888.Dec 10 2020, 7:37 AM

Added positive variants of new tests.

flx marked an inline comment as done.Dec 10 2020, 7:40 AM
flx added inline comments.
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
492

Your understanding is correct. I validated the positive cases with the existing tests from lines 104 and 112, but those didn't have any further references to the original variable. I agree it's easier to understand when the negative and positive cases are grouped together.

aaron.ballman accepted this revision.Dec 10 2020, 11:07 AM

Thanks for verifying! Aside from the minor nit with isa<>, this LGTM

This revision is now accepted and ready to land.Dec 10 2020, 11:07 AM
flx updated this revision to Diff 310997.Dec 10 2020, 12:56 PM
flx marked an inline comment as done.

Shortened isa<> expression.

This revision was landed with ongoing or failed builds.Dec 10 2020, 1:58 PM
This revision was automatically updated to reflect the committed changes.