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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko 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,
----------------
nit: *need to be removed


================
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) {
----------------
Maybe `Subregions` would fit better in this name then?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:186
+  for (const auto *Region : Regions)
+    State = removeTrackedRegions(State, Region->getBaseRegion());
+  return State;
----------------
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.


================
Comment at: clang/test/Analysis/smart-ptr.cpp:190
+/*
+// TODO: Enable this test after '=' operator overloading modeling.
+void derefAfterAssignment() {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83836