[PATCH] D99797: [analyzer] Implemented RangeSet::Factory::unite function to handle intersections and adjacency

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

I think this iteration is much better, it requires way more description as it has now.  You didn't actually describe anywhere how this algorithm actually works.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:200-201
+    }
+
+    // Build a new range.
+    while (true) {
----------------
At this point, we know for a fact that the next range we are going to add to our result set starts with `I1->From()`.  No need to check `F` for null or anything


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:210
+
+      // Check if the second range intersects or adjucent to the first one.
+      if (I1->To() < I2->From() - One)
----------------
nit: "**is** adj**a**cent"


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:212
+      if (I1->To() < I2->From() - One)
+        break;
+
----------------
Why `break`?  At this point we know that what we peaked into is actually a part of another range (in terms of the end result).
And this range is over, you just need to add it to the final result!


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:214-218
+      // We use `F` as a flag to notify that we are in a building of a new
+      // range. Set `From` of a new range if it is not set yet. If it has
+      // already been set, then we are inside this range and just looking for
+      // its end.
+      if (!F)
----------------
You actually don't need it, the moment you swap two iterators and find out that `I1->From() <= I2->From()`, that's it `I1->From()` is the beginning of the new range.  No need to carry extra flag here.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:227
+      // Now we can surely swap the iterators without any check, as we know
+      // I2-From() to be lower than I1-From().
+      std::swap(I1, I2);
----------------
 nit
Additionally, you didn't tell your readers WHY you are even swapping iterators in the first place and what relationship they have.  You need to communicate in terms of invariants.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:244
+    if (++I1 == E1)
+      goto end1;
+  };
----------------
This goto is always finishing with a return, so we can refactor `goto end1` into something like `return copyIntoResult(I1, E1);` and `goto end2` into `return copyIntoResult(I2, E2);`.
As I mentioned before, optional `F` should be removed.  On every path we should know deterministically what is the beginning and what is the end of the range that we are trying to add.


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

https://reviews.llvm.org/D99797