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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks Denys for the update! Very good! However, I think maybe we could make the code a bit more simpler.



================
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);
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:243
+    // Loop where the invariant holds.
+    while (true) {
+      // Skip all enclosed ranges.
----------------
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` ?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:290-292
+      // case 1: --[ First ]-[ First + 1 ]------>
+      // case 2: --[ First    ]-[ First + 1 ]--->
+      // -------------------[ Second + 1]------->
----------------
Should this be like in the code suggestion? You incremented `First` at L276.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:293
+      // -------------------[ Second + 1]------->
+      // In any case First + 1 goes after Second.
+      // Make sure that the loop invariant holds.
----------------
?


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

https://reviews.llvm.org/D99797