[PATCH] D86293: [analyzer] Add modeling of Eq operator in smart ptr

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:351
 
+bool SmartPtrModeling::handleEqOp(const CallEvent &Call,
+                                  CheckerContext &C) const {
----------------
vsavchenko wrote:
> xazax.hun wrote:
> > I'd prefer to call this AssignOp to avoid confusion with `==`. While your naming is correct, I always found this nomenclature confusing.
> IMO it's not a question of preference with this one, `EqOp` is misleading.
Changed to AssignOp


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:384
+
+  const auto *ArgRegionVal = State->get<TrackedRegionMap>(ArgRegion);
+  if (ArgRegionVal) {
----------------
xazax.hun wrote:
> I also find the names of the variables confusing.
> 
> Instead of `ArgRegion` what about `OtherSmartPtrRegion`?
> Instead of `ArgRegionVal` what about `OtherInnerPtr`?
Changed to `OtherSmartPtrRegion ` and `OtherInnerPtr`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:391
+
+    C.addTransition(State, C.getNoteTag([ThisRegion, ArgRegion, IsArgValNull](
+                                            PathSensitiveBugReport &BR,
----------------
xazax.hun wrote:
> Adding return after every `addTransition` is a good practive that we should follow.
Added return


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:399
+        ArgRegion->printPretty(OS);
+        OS << " is null after moved to ";
+        ThisRegion->printPretty(OS);
----------------
vsavchenko wrote:
> The grammar seems off in this one.  I think it should be smith like "after being moved to" or "after it was moved to".
Changed to "after being moved to"


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:419
+                                                    llvm::raw_ostream &OS) {
+          if (&BR.getBugType() != smartptr::getNullDereferenceBugType() ||
+              !BR.isInteresting(ArgRegion))
----------------
xazax.hun wrote:
> Isn't this the same as the beginning of the note tag above?
> 
> I wonder if there is a way to deduplicate this code. Not a huge priority though as I do not have an immediate idea for doing this in a clean way.
Yes.
The check for bug type is duplicated across all the note tags.


================
Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:132
+  std::unique_ptr<A> PToMove; // expected-note {{Default constructed smart pointer 'PToMove' is null}}
+  P = std::move(PToMove); // expected-note {{Smart pointer 'P' is null after a null value moved from 'PToMove'}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
----------------
vsavchenko wrote:
> NoQ wrote:
> > I suggest: `Null pointer value move-assigned to 'P'`.
> +1
Updated to `Null pointer value move-assigned to 'P'`


================
Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:139
+  std::unique_ptr<A> P;
+  P = std::move(PToMove); // expected-note {{Smart pointer 'PToMove' is null after moved and assigned to 'P'}}
+  PToMove->foo(); // expected-warning {{Dereference of null smart pointer 'PToMove' [alpha.cplusplus.SmartPtr]}}
----------------
NoQ wrote:
> I suggest: `Smart pointer 'PToMove' is null; previous value moved to 'P'`. Or maybe instead keep the note that the move-checker currently emits?
Going with first option "Smart pointer 'PToMove' is null; previous value moved to 'P'"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86293