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

[Valeriy Savchenko via Phabricator via cfe-commits]

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 isStdSmartPtr(const CXXRecordDecl *RD) {
+  if (!RD || !RD->getDeclContext()->isStdNamespace())
     return false;
----------------
That's great!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:188-226
+    // Check the first arg, if it is of std::unique_ptr type.
+    assert(Call.getNumArgs() == 2 && "std::swap should have two arguments");
+    const Expr *FirstArg = Call.getArgExpr(0);
+    if (!smartptr::isStdSmartPtr(FirstArg->getType()->getAsCXXRecordDecl())) {
+      return false;
+    }
+    const MemRegion *FirstArgThisRegion = Call.getArgSVal(0).getAsRegion();
----------------
Maybe a separate method then?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:217
+          if (&BR.getBugType() != smartptr::getNullDereferenceBugType() ||
+              !BR.isInteresting(FirstArgThisRegion))
+            return;
----------------
Wait, and what if the second argument is interesting?


================
Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:80
 void derefOnStdSwappedNullPtr() {
   std::unique_ptr<A> P; // expected-note {{Default constructed smart pointer 'P' is null}}
   std::unique_ptr<A> PNull; // expected-note {{Default constructed smart pointer 'PNull' is null}}
----------------
I know that this case existed before, but can we initialize `P` with non-null, so that the warning is a bit more life-like?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104300/new/

https://reviews.llvm.org/D104300