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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Oof, it took me quite some time to come back to this.
I don't think that I'll be able to gather more performance-related data than I already did, so if it's OK with y'all, we can land it.
@NoQ @steakhal @ASDenysPetrov ?



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:77-78
+  //
+  //   * Range sets are usually very simple, 1 or 2 ranges.
+  //     That's why llvm::ImmutableSet is not perfect.
+  //
----------------
ASDenysPetrov wrote:
> That's very usefull description for reviewers, but I think is redundant for newcomers, when you're reading this you can't understand why it compares to `ImmutableSet` at all. I think this preamble only relates to the Summary of this patch as the core reason of this change.
> You can just mention the fact that formerly it was an `ImmutableSet`. See //http// for details.
I don't want to fully remove this comment because this is a comment explaining implementation detail, which I believe newcomers can skip altogether.
However, I will change it to say that it was formerly an immutable set.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:209
+  private:
+    /// Return a persistent version of the given container.
+    RangeSet makePersistent(ContainerType &&From);
----------------
steakhal wrote:
> 
That's a bit too verbose for the header IMO.


================
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);
----------------
steakhal wrote:
> 
The same as above.  I don't want to mention caches or arenas for the sake of hiding those as implementation detail.  One might rightfully argue that having these methods here also leaks implementation detail into the header file, but without a pImpl, which I believe is not very widespread in the LLVM codebase, it's not possible to hide these.


================
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.
----------------
steakhal wrote:
> It might be overkill, but we can do this using transform.
Yeah, I'd say that it is wordier.


================
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
----------------
steakhal wrote:
> Hmm, we could simplify this further, assuming `Range::operator<<` is defined.
I think it looks a bit clearer with `llvm::interleaveComma`


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:105
+  void checkNegate(RawRangeSet RawOriginal, RawRangeSet RawExpected) {
+    wrap(&Self::checkNegateImpl, RawOriginal, RawExpected);
+  }
----------------
ASDenysPetrov wrote:
> Explain, please, what's the matter to use `wrap` everywhere, not just call `checkNegateImpl` explicitly?
> Won't the call works and looks the same? Like:
> ```
> this->checkNegateImpl({{MIN, A}}, {{MIN, MIN}, {D, MAX}});
> ```
Sure, we want to use a "raw" version of ranges and range sets everywhere in the tests for the simplicity of the test.  Here I got bored writing `from(Arg0), from(Arg1), ...` and went with this.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:185
+
+using IntTypes = ::testing::Types<int8_t, uint8_t, int16_t, uint16_t, int32_t,
+                                  uint32_t, int64_t, uint64_t>;
----------------
ASDenysPetrov wrote:
> Thank you for making it in a proper way, instead of a list with different type instantiations.
Oh yes, it would've been a true nightmare repeating every test for every type


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86465