[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

Overall looks good, some nits inline. Let's run it on some projects to exercise this change.



================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:27
 
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+typedef llvm::ImmutableSet<SymbolRef> PtrSet;
+
----------------
I think we should use `using` now instead of `typedef`.


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+      NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+      if (!NewSet.isEmpty()) {
+        State = State->set<RawPtrMap>(ObjRegion, NewSet);
----------------
Is it possible here the set to be empty? We just added a new element to it above. Maybe turn this into an assert or just omit this if it is impossible?


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:138
       const Expr *Origin = Call.getOriginExpr();
-      State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
-      State = State->remove<RawPtrMap>(TypedR);
+      const PtrSet *PS = State->get<RawPtrMap>(ObjRegion);
+      for (const auto &Symbol : *PS)
----------------
Using both contains and get will result in two lookups. Maybe it would be better to just use get and check if the result is nullptr?


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:159
     }
+    if (State->contains<RawPtrMap>(Entry.first)) {
+      const PtrSet *OldSet = State->get<RawPtrMap>(Entry.first);
----------------
Same comment as above.


================
Comment at: test/Analysis/dangling-internal-buffer.cpp:111
 
+void multiple_symbols(bool Cond) {
+  const char *c1, *d1;
----------------
We started to use lowercase letters for variable names. Maybe this is not the best, since it is not following the LLVM coding guidelines. So I think it would be better to rename this `Cond` to start with a lowercase letter in this patch for consistency, and update the tests to follow the LLVM coding styleguide in a separate patch later.


Repository:
  rC Clang

https://reviews.llvm.org/D49057