This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] performance-unnecessary-copy-initialization: Fix false negative.
ClosedPublic

Authored by courbet on Nov 19 2021, 7:57 AM.

Details

Summary

isConstRefReturningMethodCall should be considering
CXXOperatorCallExpr in addition to CXXMemberCallExpr. Clang considers
these to be distinct (CXXOperatorCallExpr derives from CallExpr, not
CXXMemberCallExpr), but we don't care in the context of this
check.

This is important because of
std::vector<Expensive>::operator[](size_t) const.

Diff Detail

Event Timeline

courbet created this revision.Nov 19 2021, 7:57 AM
courbet requested review of this revision.Nov 19 2021, 7:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2021, 7:57 AM
flx added a comment.Nov 19 2021, 5:13 PM

Thank you for catching and fixing this!

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

Does this work if if the object argument is a pointer or a type alias? Could you add a test to confirm?

courbet added inline comments.Nov 22 2021, 1:56 AM
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
97

It does not work with a pointer. I plan to make it work in a separate cl, but first I need to fix isOnlyUsedAsConst to make more general (specifically, handle unary *).

courbet updated this revision to Diff 389184.Nov 23 2021, 6:21 AM

Canonicalize more types and add more container tests.

flx accepted this revision.Nov 23 2021, 6:40 AM
flx added inline comments.
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
212–216

Would you mind adding this test also to the file testing the exclusion of containers:

https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp

This would cover whether the type matching of pointer types works for excluded container types.

This revision is now accepted and ready to land.Nov 23 2021, 6:40 AM
courbet updated this revision to Diff 389197.Nov 23 2021, 7:19 AM

add container exclusion tests for operators.

courbet marked an inline comment as done.Nov 23 2021, 7:20 AM
flx accepted this revision.Nov 23 2021, 7:29 AM