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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Previously, I liked this. Now I love it!
The benchmarks look promising as well.

I think I can not convince you about not modifying iterators using `++`,`--`, etc. outside of //loop-expressions//. So be it :D



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:110
+public:
+  using iterator = ImplType::const_iterator;
+
----------------
Shouldn't we call this `const_iterator`?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:209
+  private:
+    /// Return a persistent version of the given container.
+    RangeSet makePersistent(ContainerType &&From);
----------------



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:211
+    RangeSet makePersistent(ContainerType &&From);
+    /// Construct a new persistent version of the given container.
+    ContainerType *construct(ContainerType &&From);
----------------



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:278
+private:
+  /* implicit */ RangeSet(ContainerType *RawContainer) : Impl(RawContainer) {}
+  /* implicit */ RangeSet(UnderlyingType Ptr) : Impl(Ptr) {}
----------------
Btw in C++20 we will have a way expressing this: `explicit(false)`


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:281-290
+  /// Pin given points to the type represented by the current range set.
+  ///
+  /// This makes parameter points to be in-out parameters.
+  /// In order to maintain consistent types across all of the ranges in the set
+  /// and to keep all the operations to compare ONLY points of the same type, we
+  /// need to pin every point before any operation.
+  ///
----------------
Aww, excellent. Thanks.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:51
   }
-};
+  void print(raw_ostream &OS) const;
 
----------------
steakhal wrote:
> We have `dump` methods all over the place. Except this.
> I propose to rename this to `dump`.
How about marking this `LLVM_DUMP_METHOD`?
Also, defining the `operator<<` would come handly later.


================
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(); }
----------------
vsavchenko wrote:
> 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.
Okay.


================
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) {
----------------
vsavchenko wrote:
> 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`
Makes sense.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:272-273
+private:
+  RangeSet(ContainerType *RawContainer) : Impl(RawContainer) {}
+  RangeSet(UnderlyingType Ptr) : Impl(Ptr) {}
 
----------------
vsavchenko wrote:
> 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.
Okay.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:326
+
+  // If we ran out of ranges in one set, but not the other,
+  // it means that those elements are definitely not in the
----------------



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:462-470
   // Negate all other ranges.
-  for (iterator e = end(); i != e; ++i) {
+  for (; It != End; ++It) {
     // Negate int values.
-    const llvm::APSInt &newFrom = BV.getValue(-i->To());
-    const llvm::APSInt &newTo = BV.getValue(-i->From());
-    // Add a negated range.
-    newRanges = F.add(newRanges, Range(newFrom, newTo));
-  }
-
-  if (newRanges.isSingleton())
-    return newRanges;
+    const llvm::APSInt &NewFrom = ValueFactory.getValue(-It->To());
+    const llvm::APSInt &NewTo = ValueFactory.getValue(-It->From());
 
+    // Add a negated range.
----------------
It might be overkill, but we can do this using transform.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:495-507
+void RangeSet::dump(raw_ostream &OS) const {
   bool isFirst = true;
-  os << "{ ";
-  for (iterator i = begin(), e = end(); i != e; ++i) {
+  OS << "{ ";
+  for (const Range &R : *this) {
     if (isFirst)
       isFirst = false;
     else
----------------
Hmm, we could simplify this further, assuming `Range::operator<<` is defined.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:322-325
+  auto swap = [&First, &FirstEnd, &Second, &SecondEnd]() {
+    std::swap(First, Second);
+    std::swap(FirstEnd, SecondEnd);
+  };
----------------
vsavchenko wrote:
> 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.
Since we don't rely on Argument Dependent Lookup in this particular case, both versions do the same.
So **it's perfectly fine as-is**.
You should interpret my previous comment in a broader sense, like a guideline.
Which is meaningful mostly in //generic// programming.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86465