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

[Deep Majumder via Phabricator via cfe-commits]

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 `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.
I was actually planning to use something like `unmarkInteresting`, but that is grammatically incorrect. `removeInterestingness` is fair enough, but a mouthful I think. As for `Uninteresting`, I think we are better off avoiding clever uses of English and stick with simple, programmer jargon.


================
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);
----------------
xazax.hun wrote:
> I wonder about the value of this assertion. Shouldn`t `Call.isCalled(StdSwapCall)` already validate the number of arguments?
About as valuable as any assertion is ;)
My reasoning is that since we are calling `Call.getArgSval(0)` and `Call.getArgSVal(1)`, we had better assert that there are two args.
As a side-note, I had a bug in my `CallDescriptor`, which was caught by this assertion


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:196-202
+    const MemRegion *FirstArgThisRegion = Call.getArgSVal(0).getAsRegion();
+    if (!FirstArgThisRegion)
+      return false;
+    const MemRegion *SecondArgThisRegion = Call.getArgSVal(1).getAsRegion();
+    if (!SecondArgThisRegion)
+      return false;
+
----------------
vsavchenko wrote:
> I guess `handleSwap` can take `SVal`s instead of `MemRegion` and we can mostly cut on this boilerplate as well.
> ```
>    return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), C);
> ```
> and
> ```
>   return handleSwap(State, IC->getCXXThisVal(), Call.getArgSVal(0), C);
> ```
Yup, okay.


================
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)) {
----------------
xazax.hun wrote:
> 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. 
Then we would need to rename it to something like `setInterestingness`.
And renaming such a widely used function would be a real pain.


================
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)) {
----------------
RedDocMD wrote:
> xazax.hun wrote:
> > 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. 
> Then we would need to rename it to something like `setInterestingness`.
> And renaming such a widely used function would be a real pain.
Yes, but I don't think refactoring them into a lambda (within a lambda) would be that nice either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104300