[PATCH] D83836: [Analyzer] Add checkRegionChanges for SmartPtrModeling

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked 2 inline comments as done.
vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:97
 
+// If a region is removed all of the subregions needs to be removed too.
+static ProgramStateRef removeTrackedRegions(ProgramStateRef State,
----------------
vsavchenko wrote:
> nit: *need to be removed
fixed


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:98
+// If a region is removed all of the subregions needs to be removed too.
+static ProgramStateRef removeTrackedRegions(ProgramStateRef State,
+                                            const MemRegion *Region) {
----------------
vsavchenko wrote:
> Maybe `Subregions` would fit better in this name then?
Changed to Subregions


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:186
+  for (const auto *Region : Regions)
+    State = removeTrackedRegions(State, Region->getBaseRegion());
+  return State;
----------------
vsavchenko wrote:
> It is not critical, but potentially we can allocate/deallocate a whole bunch of states here.  We can do the same sort of operation with the map itself (`State->get<TrackedRegionMap>()`), which still have similar problem but to a lesser degree.  Additionally, this `get<MAP_TYPE>` method is not compile-time, it searches for the corresponding map in runtime (also in a map), so you repeat that as many times as you have `Regions`.
> 
> And super-duper over-optimization note on my part: making the loop over `Regions` to be the inner-most is more cache-friendly.  It is not really critical here, but it is overall good to have an eye for things like that.
Updated `removeTrackedSubregions` for passing `TrackedRegionMap`


================
Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:962
+  operator bool() const;
+  unique_ptr<T> &operator=(unique_ptr<T> &&p);
+};
----------------
xazax.hun wrote:
> vrnithinkumar wrote:
> > added this to support  use case like `Q = std::move(P)`
> This operation should be `noexcept`: https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D
> 
> While it makes little difference for the analyzer at this point we should try to be as close to the standard as possible. If you have some time feel free to add `noexcept` to other methods that miss it :)
Added `noexcept` for all applicable methods


================
Comment at: clang/test/Analysis/smart-ptr.cpp:190
+/*
+// TODO: Enable this test after '=' operator overloading modeling.
+void derefAfterAssignment() {
----------------
vsavchenko wrote:
> Usually we simply add the test with expectations matching current state of things, but add a TODO over those particular lines. This way when you fix the issue the test will start failing and you won't forget to uncomment it.
Enabled the test and added the todo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83836