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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:351
 
+bool SmartPtrModeling::handleEqOp(const CallEvent &Call,
+                                  CheckerContext &C) const {
----------------
I'd prefer to call this AssignOp to avoid confusion with `==`. While your naming is correct, I always found this nomenclature confusing.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:384
+
+  const auto *ArgRegionVal = State->get<TrackedRegionMap>(ArgRegion);
+  if (ArgRegionVal) {
----------------
I also find the names of the variables confusing.

Instead of `ArgRegion` what about `OtherSmartPtrRegion`?
Instead of `ArgRegionVal` what about `OtherInnerPtr`?


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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:419
+                                                    llvm::raw_ostream &OS) {
+          if (&BR.getBugType() != smartptr::getNullDereferenceBugType() ||
+              !BR.isInteresting(ArgRegion))
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86293