Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.
LGTM! You packed a lot of punch into this patch, and I like it very much. I
read the code and everything looks great. I nitpicked on one thing, the
`updateTrackedRegion` function is a bit awkward, but if you place a `TODO`
there, I'm happy to have that addressed in a later patch. I don't think this is
reason to postpone the landing on this patch any further.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:202-219
+ProgramStateRef
+SmartPtrModeling::updateTrackedRegion(const CallEvent &Call, CheckerContext &C,
+ const MemRegion *ThisValRegion) const {
+ ProgramStateRef State = C.getState();
+ auto NumArgs = Call.getNumArgs();
+
+ if (NumArgs == 0) {
----------------
Hmm, this function feels clunky. So, if the call has no arguments, we set the
smart pointer to null, otherwise if its a single-argument then we set it to
whatever the argument is?
How about `operator[]`, that also takes a single argument, but isn't a memory
region? `get`, `get_deleter` don't take any arguments, but they don't set the
internal pointee to null either. The name `updateTrackedRegion` however
suggests that whatever operation was done, this is the one-tool-to-solve-it-all
function to take care of it.
I think this function handles too many things as once, and the name and lack of
documentation obfuscates its purpose. How about we put the relevant code to
`handleRelease`, and repurpose the rest of the function like this:
`updateOwnedRegion(CallEvent, CheckerContext, MemRegion of the smart pointer,
MemRegion to take ownership of)`
What do you think?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81315/new/
https://reviews.llvm.org/D81315
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
