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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
