[PATCH] D105421: [analyzer] Handle << operator for std::unique_ptr

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:218-219
+
+  if (isStdOstreamOperatorCall(Call))
+    return true;
+
----------------
NoQ wrote:
> When you're doing `evalCall` you're responsible for modeling *all* aspects of the call. One does not simply say that they modeled all aspects of the call when they didn't even set the return value :)
> 
> Similarly to how `make_unique` delegates work to the constructor of the managed object and `~unique_ptr` delegates work to the destructor of the managed object, I suspect this code could delegate work to `basic_ostream::operator<<(T *)`. We don't yet have any facilities to implement such logic yet (i.e., entering function call from a checker callback).
> 
> Given that we don't do much modeling of `basic_ostream` yet, I think it's perfectly fine to invalidate the stream entirely (which would be as precise as whatever the default evaluation gave us) (see `ProgramState::invalidateRegions`) and return the reference to the stream (which is already better than what the default evaluation gave us); additionally, we get extra precision because we don't invalidate the rest of the heap. That's the bare minimum of what we have to do here if we are to do anything at all.
> 
> This also gives some ideas of how to write tests for this patch.
> 
> That said, I suspect that this patch is not critical to enabling the checker by default, because we probably already know that this method doesn't change the inner pointer value (simply through not doing anything special) (and accepting the smart pointer by `const` reference, so even if we implement invalidation it will probably still "just work") (?).
Does this work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105421