vsavchenko added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:110 + iterator end() const { return Impl->end(); } + size_t size() const { return Impl->size(); } + ---------------- steakhal wrote: > I think it should be `std::size_t`. Yeah, I didn't see a unified stand of the community on this matter. It looks like both options are pretty widespread. I mimicked `SmallVector.h`, but I can easily change it. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:118-123 + /// Create a new set with all ranges from both LHS and RHS. + /// Possible intersections are not checked here. + /// + /// Complexity: O(N + M) + /// where N = size(LHS), M = size(RHS) + RangeSet add(RangeSet LHS, RangeSet RHS); ---------------- steakhal wrote: > What about `merge` or `combine`? I know that `union` is a keyword thus we can > not use. Maybe, but the real `union` function is not present. And I think that in the original code it is `add` to hint that it's not really a union and nothing complex is done here. Maybe something like `mergeUnchecked` would work. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:149-160 + /// Intersect the given set with the closed range [Lower, Upper]. + /// + /// Unlike the Range type, this range uses modular arithmetic, corresponding + /// to the common treatment of C integer overflow. Thus, if the Lower bound + /// is greater than the Upper bound, the range is taken to wrap around. This + /// is equivalent to taking the intersection with the two ranges [Min, + /// Upper] and [Lower, Max], or, alternatively, /removing/ all integers ---------------- steakhal wrote: > I think an example would be awesome here. Agree, I simply copy-pasted the comment, but it would be better to expand it a bit because it might be confusing. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:170-174 + /// Delete the given point from the range set. + /// + /// Complexity: O(N) + /// where N = size(From) + RangeSet deletePoint(const llvm::APSInt &Point, RangeSet From); ---------------- steakhal wrote: > Why does this operation take `O(N)` time? Shouldn't be `O(logN)` instead? We still create a copy of an old vector to maintain persistence, that's why it's `O(N)`. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:190-193 + /// And it makes us to separate the range + /// like [MIN, N] to [MIN, MIN] U [-N, MAX]. + /// For instance, whole range is {-128..127} and subrange is [-128,-126], + /// thus [-128,-127,-126,...] negates to [-128,...,126,127]. ---------------- steakhal wrote: > The same thing with this comment, I think I'll rework it even more ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:195-196 + /// + /// Negate restores disrupted ranges on bounds, + /// e.g. [MIN, B] => [MIN, MIN] U [-B, MAX] => [MIN, B]. + /// ---------------- steakhal wrote: > +1 ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:203 + private: + RangeSet makePersistent(ContainerType &&From); + ContainerType *construct(ContainerType &&From); ---------------- steakhal wrote: > As commented later, this function worth to be documented. I didn't think that it's necessary because of the fact that it's `private`, but it's pretty important, so I'll do it. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:207-208 + + RangeSet intersect(const RangeSet::ContainerType &LHS, + const RangeSet::ContainerType &RHS); + ---------------- steakhal wrote: > Why do we need to explicitly spell the `RangeSet` here? Good point! ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:210-211 + + // Many operations inlcude producing new APSInt values and that's why + // we need this factory. + BasicValueFactory &ValueFactory; ---------------- steakhal wrote: > +1 ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:213-215 + // Allocator for all the created containers. + // Containers might own their own memory and that's why it is specific + // for the type, so it calls containter destructors upon deletion. ---------------- steakhal wrote: > +1 ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:225-228 + RangeSet(const RangeSet &) = default; + RangeSet &operator=(const RangeSet &) = default; + RangeSet(RangeSet &&) = default; + RangeSet &operator=(RangeSet &&) = default; ---------------- steakhal wrote: > If this is the Rule of 5, where is the defaulted destructor? It is, and it is forgotten, good catch! ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:269 -public: - RangeSet Intersect(BasicValueFactory &BV, Factory &F, llvm::APSInt Lower, - llvm::APSInt Upper) const; - RangeSet Intersect(BasicValueFactory &BV, Factory &F, - const RangeSet &Other) const; - RangeSet Negate(BasicValueFactory &BV, Factory &F) const; - RangeSet Delete(BasicValueFactory &BV, Factory &F, - const llvm::APSInt &Point) const; + bool operator==(const RangeSet &Other) const { return *Impl == *Other.Impl; } ---------------- steakhal wrote: > If we implement equality, we should also support inequality. Sure! ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:272-273 +private: + RangeSet(ContainerType *RawContainer) : Impl(RawContainer) {} + RangeSet(UnderlyingType Ptr) : Impl(Ptr) {} ---------------- steakhal wrote: > Missing `explicit`. More like missing `/* implicit */` because it is intentional ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:277 + bool pin(llvm::APSInt &Point) const; + bool containsImpl(llvm::APSInt &Point) const; + ---------------- steakhal wrote: > If it's a //read-only// operation why don't we take by value? Just like > `contains` does. > If it has side-effects which modify the Point, why is that not documented > here? Good point. Essentially it modifies the argument the same way `pin` functions do - by changing its type. I'll add the comments ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:146-149 +RangeSet::ContainerType *RangeSet::Factory::construct(ContainerType &&From) { + void *Buffer = Arena.Allocate(); + return new (Buffer) ContainerType(std::move(From)); +} ---------------- steakhal wrote: > Off-topic: Why do the Allocate and the similar //allocator// functions return > `T*` if there is NO living object there (yet). > There will be, only if the placement constructor called... > I highly agree with you using `void *` there. 100% agree. It might be a source of mistakes where the type system doesn't help. ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:177-178 + + --It; + return It->Includes(Point); +} ---------------- steakhal wrote: > I don't really see the benefits of one way over another and as I said in another comment, I don't really like to pay for an extra copy with iterators as just a rule of thumb. ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:181-188 +bool RangeSet::pin(llvm::APSInt &Point) const { + APSIntType Type(getMinValue()); + if (Type.testInRange(Point, true) != APSIntType::RTR_Within) + return false; + + Type.apply(Point); + return true; ---------------- steakhal wrote: > Previously we had an early `empty` check here. > Why don't we have that now? > Same question for the other overload. I'll add assertions checking for non-emptiness. Essentially, it is a private function and the callers ensure this precondition. ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:322-325 + auto swap = [&First, &FirstEnd, &Second, &SecondEnd]() { + std::swap(First, Second); + std::swap(FirstEnd, SecondEnd); + }; ---------------- steakhal wrote: > It could definitely bear a longer name. I think that there is nothing wrong in spelling out `std::swap` twice. And do we have a naming convention for lambdas, is it a variable or a function from a naming perspective? ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:367-370 + if (Second->To() > First->To()) + // Here we make a decision to keep First as the "longer" + // range. + swap(); ---------------- steakhal wrote: > OK ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:375 + // ---- First ]--------------------> + // ---- Second ]--[ ++Second ----------> + // ---------------- steakhal wrote: > It's unfortunate to refer to the next subrange by the `++Second`. > Since `operator++` has a //side-effect// I would recommend the `Second+1` > instead. NP ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:384 + Result.push_back(Range(IntersectionStart, Second->To())); + ++Second; + // ...and that the invariant will hold for a valid ++Second ---------------- steakhal wrote: > It's probably a personal taste though. > I think better to use the more verbose way incrementing an iterator if you > are in the body of the loop. > It catches more attention. It basically makes it `Second++`, and even though for these iterators it doesn't really matter I prefer not to pay extra copy when I don't need it. ================ Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:498 os << "{ "; for (iterator i = begin(), e = end(); i != e; ++i) { if (isFirst) ---------------- steakhal wrote: > Can we use range-based for loops? We sure can, I wanted to change it but forgot Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86465/new/ https://reviews.llvm.org/D86465 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits