[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG0cd98bef1b6f: [analyzer] Handle std::swap for std::unique_ptr (authored by RedDocMD). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

RedDocMD updated this revision to Diff 359600. RedDocMD added a comment. Post rebase cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104300/new/ https://reviews.llvm.org/D104300 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h clang/l

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

balazske added a comment. This change is accepted already. Is it good to commit? I would need the change for symbol "uninterestingness" in D104925 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104300/new/ https:

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

RedDocMD updated this revision to Diff 353068. RedDocMD added a comment. Some more refactoring Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104300/new/ https://reviews.llvm.org/D104300 Files: clang/include/clang/StaticAnalyzer/Core/BugReporter/

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

RedDocMD marked 5 inline comments as done. RedDocMD added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:435 + void markNotInteresting(SymbolRef sym); + xazax.hun wrote: > Bikeshedding: I wonder if we prefer `Un

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I think this patch is good to go as long as other folks are happy. I'm glad that uninterestingness is becoming a thing! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

xazax.hun added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:435 + void markNotInteresting(SymbolRef sym); + Bikeshedding: I wonder if we prefer `Uninteresting` to `NotInteresting`. Or alternatively, if we wa

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

vsavchenko added a comment. I agree with @NoQ that notes are pretty much straightforward and we shouldn't abandon them altogether. Refinements about what is null or non-null pointer are purely cosmetic and we definitely can tweak this messaging. Comment at: clang/lib/StaticA

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

RedDocMD updated this revision to Diff 352627. RedDocMD added a comment. Marking and un-marking interestingness Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104300/new/ https://reviews.llvm.org/D104300 Files: clang/include/clang/StaticAnalyzer/

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

NoQ added a comment. I think notes are fairly straightforward: if we're tracking one smart pointer below the swap, we should stop tracking it and start tracking the other smart pointer above the swap. If both were tracked, keep tracking both (but in swapped manners, assuming we're tracking them

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

RedDocMD added a comment. I have gotten rid of notes for the time being. The trouble is that, we were not figuring out whether the null-pointer de-reference was occurring due to the null value being swapped with the current pointer. We just //assumed// that. So I am guessing we would need a visi

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

RedDocMD updated this revision to Diff 352201. RedDocMD added a comment. Refactored common code, removed note emission Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104300/new/ https://reviews.llvm.org/D104300 Files: clang/lib/StaticAnalyzer/Che

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

RedDocMD added a comment. The current implementation of how notes are emitted in `handleSwap` is broken. Consider the following code: #include void foo() { auto ptr1 = std::unique_ptr(new int(10)); auto ptr2 = std::unique_ptr(new int(13)); ptr1.swap(ptr2); ptr1.reset();

[PATCH] D104300: [analyzer] Handle std::swap for std::unique_ptr

RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:217 + if (&BR.getBugType() != smartptr::getNullDereferenceBugType() || + !BR.isInteresting(FirstArgThisRegion)) +return; vsavchenk

[PATCH] D104300: [analyzer] Handle `std::swap`

vsavchenko added a comment. It looks like new functionality is VERY much like `handleSwap`, and we should definitely merge common parts. Also, I think that commit title should be more specific than "Handle std::swap". It should at least mention the checker. Repository: rG LLVM Github Monor

[PATCH] D104300: [analyzer] Handle `std::swap`

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:94-103 const auto *RecordDecl = MethodDecl->getParent(); - if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace()) + return isStdSmartPtr(RecordDecl); +} + +bool

[PATCH] D104300: [analyzer] Handle `std::swap`

RedDocMD added a comment. I am not entirely satisfied with the note that is being emitted currently from the `std::swap` handling (its ripped off from the note emitted for `std::unique_ptr::swap`). Any suggestions? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revi

[PATCH] D104300: [analyzer] Handle `std::swap`

RedDocMD created this revision. RedDocMD added reviewers: NoQ, vsavchenko, teemperor, xazax.hun. Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware. RedDocMD requested review of this re