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

[Artem Dergachev via Phabricator via cfe-commits]

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