NoQ added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:218-219
+
+  if (isStdOstreamOperatorCall(Call))
+    return true;
+
----------------
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") (?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105421

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to