[PATCH] D86465: [analyzer][solver] Redesign constraint ranges data structure

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D86465#2238614 <https://reviews.llvm.org/D86465#2238614>, @martong wrote:

> These are really promising figures, nice work! (And the measurement stuff itself is also a great addition, thanks for that!)

Thanks 😊

> Not that it would matter much, but I was just wondering why there is a slightly bigger memory usage in some of the `docker/small` projects? The most conspicuous is `oatpp`.

The measurements fluctuate quite significantly and as you can see the means for both `old` and `new` experiments for `oatpp` are pretty close.  So, I would say that the memory differences are not conclusive.
This being said, the performance measurements should also be taken with a grain of salt.  We can't really say that we get **5-10%** performance improvement for sure, but that analysis //tends// to be faster instead.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:53-55
+  // In order to keep non-overlapping ranges sorted, we can compare only From
+  // points.
+  inline bool operator<(const Range &RHS) const { return From() < RHS.From(); }
----------------
steakhal wrote:
> It's a good practice to define comparison operators as //friend// functions inline.
> Even if we don't rely on implicit conversions.
It doesn't seem like there is a single opinion on this in the codebase.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:135
+    /// @{
+    RangeSet getSet(Range Origin);
+    RangeSet getSet(const llvm::APSInt &From, const llvm::APSInt &To) {
----------------
steakhal wrote:
> NoQ wrote:
> > "But what about `setGet()`???" - some user, probably :)
> Why don't we call this `createSetOf`?
> And `createEmptySet`.
> I know that we don't create the empty set, but then what does a //factory// if not create stuff?
The naming is this way to be consistent with `ImmutableSet::Factory`


================
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:
> vsavchenko wrote:
> > steakhal wrote:
> > > Missing `explicit`.
> > More like missing `/* implicit */` because it is intentional
> It doesn't have a too long identifier.
> The users can always refer to it `auto R = RangeSet(...)`, so we still don't repeat ourselves.
> Do you have any particular counterexample?
> Probably the tests will become slightly more bloated but eh. whatever.
These constructors are `private` and used in `Factory` methods.  IMO they make those methods less cluttered.


================
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:
> vsavchenko wrote:
> > 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?
> That's a good question :)
> I would say that its a variable, since you can mark it `const`, otherwise you could overwrite it. xD But that's a different story I think.
> About the `swap` thingie, its a good practice to respect ADL for functions which know to be used via ADL.
> Even if we don't depend on ADL in this particular case.
I'm sorry but I still don't get the importance of dropping `std::` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86465