vrnithinkumar added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:198
void SmartPtrModeling::handleSwap(const CallEvent &Call,
CheckerContext &C) const {
----------------
vsavchenko wrote:
> I think it would be good to add some comments in the body of the function to
> be more explicit about the situation you are handling.
Added comments
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:224-228
+ if (ThisRegionInnerPointerVal) {
+ State = State->set<TrackedRegionMap>(ArgRegion,
*ThisRegionInnerPointerVal);
+ } else {
+ State = State->remove<TrackedRegionMap>(ArgRegion);
+ }
----------------
vsavchenko wrote:
> These two if's are clearly duplicates and it seems like a good candidate for
> a separate function.
Thanks!
created a separate function `updateSwappedRegion()`
================
Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:964-965
+
+ template <typename T>
+ void swap(unique_ptr<T> &x, unique_ptr<T> &y) noexcept {
+ x.swap(y);
----------------
xazax.hun wrote:
> NoQ wrote:
> > You seem to be relying on the fact that global `std::swap` is implemented
> > in terms of the member `std::swap`. That's an implementation detail of the
> > standard library; i'm not sure that this is always the case. Ideally we
> > should model the global `std::swap` separately.
> I am not sure how reliable cppreference is, but I think this overload might
> actually be guaranteed by the standard:
> https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2
I also made the assumption based on this
(https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2).
================
Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:965
+ template <typename T>
+ void swap(unique_ptr<T> &x, unique_ptr<T> &y) noexcept {
+ x.swap(y);
----------------
xazax.hun wrote:
> Maybe it is worth to add a TODO to introduce an additional template parameter
> once the deleter is added above.
>
> https://en.cppreference.com/w/cpp/memory/unique_ptr/swap2
Added the TODO
================
Comment at: clang/test/Analysis/smart-ptr.cpp:124
+
+void derefOnStdSwappedNullPtr() {
+ std::unique_ptr<A> P(new A());
----------------
vsavchenko wrote:
> xazax.hun wrote:
> > This test case is very similar to the first one. Maybe you could move them
> > closer. And you could make both pointers invalid to make them more distinct.
> +1
Moved and made both pointers invalid in the test
================
Comment at: clang/test/Analysis/smart-ptr.cpp:131
+}
\ No newline at end of file
----------------
vrnithinkumar wrote:
> I will fix this.
Fixed
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83877/new/
https://reviews.llvm.org/D83877
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
