[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hi Henry. I had a quick look at the patch, here are some remarks.



================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2092
+      // 'invalidateRegions()', which will remove the <MemRegion, SVal> pair
+      // in CStringLength map. So calls 'InvalidateBuffer()' after getting
+      // old string length and before setting the new string length.
----------------
"So we call"?


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2100
+    std::tie(StateNullChar, StateNonNullChar) =
+        assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+
----------------
I think we can use the argument type here (which is int).
One more question. Could we use `SVal::isZeroConstant()` here instead? Do we need the path-sensitivity for the value or we can just check if the value is a constant zero?


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2103
+    if (StateNullChar && !StateNonNullChar) {
+      // If the value of the second arguement of 'memset()' is zero, set the
+      // string length of destination buffer to 0 directly.
----------------
"argument"


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2127
+                             /*IsSourceBuffer*/ false, Size);
+  }
+
----------------
Could you please factor this chunk to a function? It makes the method too big.


================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:148
+  ProgramStateRef NewState = makeWithStore(NewStore);
+  return Mgr.getOwningEngine()
+             ? Mgr.getOwningEngine()->processRegionChange(NewState, R, LCtx)
----------------
Do clients of `overwriteRegion` really need to call checkers?


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:446
+  // default binding.
+  StoreRef OverwriteRegion(Store store, const MemRegion *R, SVal V) override {
+    RegionBindingsRef B = getRegionBindings(store);
----------------
LLVM naming conventions say that function names should start with small letter.


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1718
 
-    llvm_unreachable("Unknown default value");
+    return val;
   }
----------------
Could you please explain what is the reason of this change? Do we get new value kinds?


================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:72
 
+StoreRef StoreManager::OverwriteRegion(Store store, const MemRegion *R, SVal V) {
+  return StoreRef(store, *this); 
----------------
Could we make this method pure virtual?


Repository:
  rC Clang

https://reviews.llvm.org/D44934