[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:147
 
-    if (!move::isMovedFrom(State, ThisR)) {
-      // TODO: Model this case as well. At least, avoid invalidation of
-      // globals.
-      return false;
+    if (ModelSmartPtrDereference) {
+      handleBoolOperation(Call, C);
----------------
xazax.hun wrote:
> vrnithinkumar wrote:
> > This seems little messy here.
> > I guess once we model the `std::move` for smart ptr it will be less messy 
> I agree. `isNullAfterMoveMethod` is especially confusing as it does not do what the name of the function says. It checks if the `Call` is a boolean conversion operator. I'd suggest renaming that method to make this code a bit more sensible. 
> 
> Moreover, when `ModelSmartPtrDereference` is true, we no longer model moves below right? I think a comment that this logic is duplicated `handleBoolOperation` might make the code cleaner. 
> 
> But yeah, this needs to be cleaned up, hopefully really soon.
- Renamed the method to `isBooleanOpMethod`
- Added comments and FIXME
Hopefully we can clean up once `std::move` is modeled.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:389
+    }
+    C.addTransition(State);
+  } else if (move::isMovedFrom(State, ThisRegion)) {
----------------
xazax.hun wrote:
> This looks fine for now, but we often prefer adding a return after each case to avoid executing multiple `addTransition`s accidentally after refactoring.
Added return


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:411
+      if (NotNullState) {
+        auto NonNullVal = C.getSValBuilder().makeTruthVal(true);
+        NotNullState =
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > Since the inner pointer value can be any non-null value, I am not sure what should be the value to be added to the map for tracking.
> > 
> Don't add values, constrain existing values.
Made changes to create the conjured symbol and use that to constrain. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86027