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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
