vrnithinkumar marked 2 inline comments as done. vrnithinkumar added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:97 +// If a region is removed all of the subregions needs to be removed too. +static ProgramStateRef removeTrackedRegions(ProgramStateRef State, ---------------- vsavchenko wrote: > nit: *need to be removed fixed ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:98 +// If a region is removed all of the subregions needs to be removed too. +static ProgramStateRef removeTrackedRegions(ProgramStateRef State, + const MemRegion *Region) { ---------------- vsavchenko wrote: > Maybe `Subregions` would fit better in this name then? Changed to Subregions ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:186 + for (const auto *Region : Regions) + State = removeTrackedRegions(State, Region->getBaseRegion()); + return State; ---------------- vsavchenko wrote: > It is not critical, but potentially we can allocate/deallocate a whole bunch > of states here. We can do the same sort of operation with the map itself > (`State->get<TrackedRegionMap>()`), which still have similar problem but to a > lesser degree. Additionally, this `get<MAP_TYPE>` method is not > compile-time, it searches for the corresponding map in runtime (also in a > map), so you repeat that as many times as you have `Regions`. > > And super-duper over-optimization note on my part: making the loop over > `Regions` to be the inner-most is more cache-friendly. It is not really > critical here, but it is overall good to have an eye for things like that. Updated `removeTrackedSubregions` for passing `TrackedRegionMap` ================ Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:962 + operator bool() const; + unique_ptr<T> &operator=(unique_ptr<T> &&p); +}; ---------------- xazax.hun wrote: > vrnithinkumar wrote: > > added this to support use case like `Q = std::move(P)` > This operation should be `noexcept`: > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D > > While it makes little difference for the analyzer at this point we should try > to be as close to the standard as possible. If you have some time feel free > to add `noexcept` to other methods that miss it :) Added `noexcept` for all applicable methods ================ Comment at: clang/test/Analysis/smart-ptr.cpp:190 +/* +// TODO: Enable this test after '=' operator overloading modeling. +void derefAfterAssignment() { ---------------- vsavchenko wrote: > Usually we simply add the test with expectations matching current state of > things, but add a TODO over those particular lines. This way when you fix the > issue the test will start failing and you won't forget to uncomment it. Enabled the test and added the todo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83836/new/ https://reviews.llvm.org/D83836 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits