[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

This revision was automatically updated to reflect the committed changes. Closed by commit rL360540: [clang-tidy] new check: bugprone-unhandled-self-assignment (authored by ztamas, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: ht

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas added a comment. > I definitely want to see the diagnostic text changed away from "might be" -- > that's too noncommittal for my tastes. I'd also like to see the additional > test case (I suspect it will just work out of the box, but if it doesn't, it > would be good to know about it). T

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 198789. ztamas added a comment. copy operator -> copy assignment operator Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 Files: clang-tools-extra/clang-tidy/bugprone/

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 198787. ztamas added a comment. Changed "might not" to "does not" in the warning message. Added the requested test case with the swapped template arguments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

aaron.ballman added a comment. In D60507#1491095 , @ztamas wrote: > Ping. > Is it good to go or is there anything else I need to include in this patch? Sorry for the delay, I was gone for WG14 meetings last week, which made reviewing more involved patc

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas added a comment. Ping. Is it good to go or is there anything else I need to include in this patch? Among the lots of idea, I'm not sure which is just brainstorming and which is a change request. The check seems to work (has useful catches and does not produce too many false positives), ho

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 197014. ztamas added a comment. Better handling of template and non-template self-copy Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 Files: clang-tools-extra/clang-t

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas added a comment. Ok, is there anything else should be included in this patch? I saw more ideas about how to extend the functionality (custom pointers, fix-it, etc). I interpreted these ideas as nice-to-have things for the future. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 196410. ztamas added a comment. Remove outdated comment from docs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 Files: clang-tools-extra/clang-tidy/bugprone/Bugpron

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked 8 inline comments as done. ztamas added a comment. Mark some comments Done, which were handled some way. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 _

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas added a comment. I also reformatted the code with clang-format. So now the templates are handled, however, it's still not fit with the cert rule because we not catch all classes, but only those who have suspicious fields. I think this one can be added in a follow-up patch and then this c

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 196295. ztamas added a comment. Make the check to work with templates too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 Files: clang-tools-extra/clang-tidy/bugprone/

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked 3 inline comments as done. ztamas added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:62-64 + const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl( + has(fieldDecl(anyOf(hasType(pointerT

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:54 + recordType(hasDeclaration(classTemplateSpecializationDecl( + hasAnyName("std::shared_ptr", "std::unique_ptr", "std::weak_ptr", +

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas added a comment. Ok, so I removed the alias from cert module and added CERT rule link as a "see also". So I think we solved the problem that things do not conform with the CERT requirements and can focus on the actual problem. Summary, templates are still ignored. If there is any idea ho

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked 5 inline comments as done. ztamas added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-36 + const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant( + binaryOperator(anyOf(hasOperatorName("=="), h

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 196279. ztamas added a comment. Added missing fullstop Added a new test cases making sure that HasNoSelfCheck works correctly Added template instantiation to tests Remove the alias from cert module and add the CERT link as a see also Repository: rG LLVM Git

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked an inline comment as done. ztamas added inline comments. Comment at: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351 + int *p; +}; aaron.ballman wrote: > ztamas wrote: > > aaron.ballman wrote: > > > ztamas wrote: > > >

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked 4 inline comments as done. ztamas added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-36 + const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant( + binaryOperator(anyOf(hasOperatorName("=="), h

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

aaron.ballman marked an inline comment as done. aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-36 + const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant( + binaryOperator(anyOf(hasOperato

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 196066. ztamas added a comment. Add false positive test cases. Added a note about template related limitation to the docs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked 2 inline comments as done. ztamas added inline comments. Comment at: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351 + int *p; +}; aaron.ballman wrote: > ztamas wrote: > > JonasToth wrote: > > > Please add tests with t

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked 4 inline comments as done. ztamas added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6 + +`cert-oop54-cpp` redirects here as an alias for this check. + aaron.ballman wrote: > You shoul

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 195382. ztamas added a comment. Update patch based on reviewer comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 Files: clang-tools-extra/clang-tidy/bugprone/B

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:130-131 "bugprone-throw-keyword-missing"); +CheckFactories.registerCheck( +"bugprone-too-small-loop-variable"); CheckFactories.registerCheck( --

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

xazax.hun added inline comments. Comment at: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:326 + +// We should not catch move assignment operators. +class MoveAssignOperator { While I do agree move assignment operators should not be ch

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked an inline comment as done. ztamas added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:51 + // Matcher for standard smart pointers. + const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType( + r

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

xazax.hun added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:51 + // Matcher for standard smart pointers. + const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(classTemplate

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 195061. ztamas added a comment. Missed to syncronize ReleasNotes with the corresponding doc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 Files: clang-tools-extra/c

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 194925. ztamas marked an inline comment as done. ztamas added a comment. Documentation fixes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60507/new/ https://reviews.llvm.org/D60507 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas marked 13 inline comments as done. ztamas added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10 + +This check corresponds to the CERT C++ Coding Standard rule +`OOP54-CPP. Gracefully handle self-copy assignme

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6 + +"cert-oop54-cpp" redirects here as an alias for this check. + Please use back-tick to highlight cert-oop54-cpp. =

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas updated this revision to Diff 194921. ztamas added a comment. Updated the code based on reviewer comments: - Alphabetical order in `BugproneTidyModule.cpp` - Handling of `auto_ptr` - Test cases for template classes (I made the check ignore these case otherwise this can lead to false posit

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas added a comment. I'll update the patch based on the comments. Just a note about the false positive ratio. In LibreOffice we run other static analyzers too, that's why there were less positive catches there than false positives. For example, PVS studio also catches unprotected copy assignm

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

aaron.ballman added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10 + +This check corresponds to the CERT C++ Coding Standard rule +`OOP54-CPP. Gracefully handle self-copy assignment Eugene.Zelenko

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:20 +void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) { + + // We don't care about deleted, default or implicit operator implementatio

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

alexfh added a comment. Thanks for the useful check! I have a few comments, see inline. Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-35 + binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")), + any

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

JonasToth added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:102 "bugprone-too-small-loop-variable"); +CheckFactories.registerCheck( +"bugprone-unhandled-self-assignment"); please order this list a

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas added a comment. On LibreOffice source code I found 36 catches with this check. 16 of them was worth to fix because in those cases the object state was changed in some way after a self-assignment: https://cgit.freedesktop.org/libreoffice/core/commit/?id=3a5d78365dd172881c16c03e67f2d170ffc6

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas added a comment. On llvm source code the check finds three suspicious methods: /home/zolnai/libreoffice/clang/llvm-project/clang/lib/AST/NestedNameSpecifier.cpp:534:1: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] operator=(const

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

ztamas created this revision. Herald added subscribers: cfe-commits, xazax.hun, mgorny. Herald added a project: clang. This check searches for copy assignment operators which might not handle self-assignment properly. There are three patterns of handling a self assignment situation: self check, c