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

[Gábor Horváth via Phabricator via cfe-commits]

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 want to emphasize this is only the lack of interestingness, we could name it `removeInterestingness`. I do not have strong feelings about any of the options, I was just wondering if any of you has a preference.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:191
+    // 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);
----------------
I wonder about the value of this assertion. Shouldn`t `Call.isCalled(StdSwapCall)` already validate the number of arguments?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:193
+    const Expr *FirstArg = Call.getArgExpr(0);
+    if (!smartptr::isStdSmartPtr(FirstArg->getType()->getAsCXXRecordDecl())) {
+      return false;
----------------
Nit: we usually omit braces for simple `if` statements.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:458-467
+    if (BR.isInteresting(FirstThisRegion) &&
+        !BR.isInteresting(SecondThisRegion)) {
+      BR.markInteresting(SecondThisRegion);
+      BR.markNotInteresting(FirstThisRegion);
+    }
+    if (BR.isInteresting(SecondThisRegion) &&
+        !BR.isInteresting(FirstThisRegion)) {
----------------
vsavchenko wrote:
> nit: these two pieces of code are very much the same
I guess we could make `markInteresting` take an optional bool and swap the interestingness unconditionally. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104300