NoQ added inline comments.
================
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) {
----------------
Szelethus wrote:
> 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?
Yup, I completely agree. I think this structure will naturally evolve into
something cleaner once more modeling gets added.
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
