[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

Mon Jul 6 12:53:38 PDT 2020

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