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

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked an inline comment as done.
vrnithinkumar 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) {
----------------
NoQ wrote:
> 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.
Thanks for the detailed comment.

I totally agree this method is handling many things. 
For methods like `get`, `get_deleter` this makes no sense at all.

I will add TODO there,  will address this  in a later patch once we get a clear picture after more modeling added.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81315/new/

https://reviews.llvm.org/D81315