[PATCH] D83877: [Analyzer] Handle unique_ptr::swap() in SmartPtrModeling

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:198
 
 void SmartPtrModeling::handleSwap(const CallEvent &Call,
                                   CheckerContext &C) const {
----------------
vsavchenko wrote:
> I think it would be good to add some comments in the body of the function to be more explicit about the situation you are handling.
Added comments


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:224-228
+  if (ThisRegionInnerPointerVal) {
+    State = State->set<TrackedRegionMap>(ArgRegion, *ThisRegionInnerPointerVal);
+  } else {
+    State = State->remove<TrackedRegionMap>(ArgRegion);
+  }
----------------
vsavchenko wrote:
> These two if's are clearly duplicates and it seems like a good candidate for a separate function.
Thanks!
created a separate function `updateSwappedRegion()`


================
Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:964-965
+
+  template <typename T>
+  void swap(unique_ptr<T> &x, unique_ptr<T> &y) noexcept {
+    x.swap(y);
----------------
xazax.hun wrote:
> NoQ wrote:
> > You seem to be relying on the fact that global `std::swap` is implemented in terms of the member `std::swap`. That's an implementation detail of the standard library; i'm not sure that this is always the case. Ideally we should model the global `std::swap` separately.
> I am not sure how reliable cppreference is, but I think this overload might actually be guaranteed by the standard: https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2
I also made the assumption based on this (https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2).



================
Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:965
+  template <typename T>
+  void swap(unique_ptr<T> &x, unique_ptr<T> &y) noexcept {
+    x.swap(y);
----------------
xazax.hun wrote:
> Maybe it is worth to add a TODO to introduce an additional template parameter once the deleter is added above.
> 
> https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2
Added the TODO


================
Comment at: clang/test/Analysis/smart-ptr.cpp:124
+
+void derefOnStdSwappedNullPtr() {
+  std::unique_ptr<A> P(new A());
----------------
vsavchenko wrote:
> xazax.hun wrote:
> > This test case is very similar to the first one. Maybe you could move them closer. And you could make both pointers invalid to make them more distinct.
> +1
Moved and made both pointers invalid in the test


================
Comment at: clang/test/Analysis/smart-ptr.cpp:131
+}
\ No newline at end of file

----------------
vrnithinkumar wrote:
> I will fix this.
Fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83877