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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:229-233
+    // We want to keep the following invariant at all times:
+    // ---[ First ------>
+    // -----[ Second --->
+    if (First->From() > Second->From())
+      swapIterators(First, FirstEnd, Second, SecondEnd);
----------------
martong wrote:
> I am not sure about this, but perhaps we could put this `swapIterators` call right at the beginning of the nested `while` loop (L243). That would eliminate the need to call it again at the end of the second while loop.
I'm afraid, it is not the case. Every loop needs its own `swapIterators`. As you can see, `swapIterators` in the nested loop invokes not always because of `break;` in the middle. Otherwise, it would invokes each time. But I checked your suggestion. It doesn't work.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:243
+    // Loop where the invariant holds.
+    while (true) {
+      // Skip all enclosed ranges.
----------------
martong wrote:
> So, this loop is about to merge conjunct Ranges. The first time when we find disjoint Ranges then we break out. (Or we return once we reach the end of any of the RangeSets.)
> This makes we wonder, if it would be possible to split this `while` loop into a lambda named `mergeConjunctRanges` ?
I'm not in favor of introducing another level of abstraction. It would divide the flow/comments and could reduce readability and performance which is crucial here.


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

https://reviews.llvm.org/D99797