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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

I think, the visual comments that we have in `intersect` makes the code of `intersect` a lot easier to follow. Could you please add similar visual comments here? Also, I find `First` and `Second` in `intersect` way more better naming than `I1` and `I2`, could you please update?



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:169
+
+  iterator I1 = LHS.begin();
+  iterator E1 = LHS.end();
----------------
I think, the naming conventions we have in `intersect` are easier to follow. Could we call `I1` as `First` and `I2` as `Second`?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:181
+  // Append the rest of the ranges from another range set to the Result
+  // and return the later.
+  auto AppendRest = [&Result](iterator I, iterator E) {
----------------



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:188
+  // Handle a corner case first when both range sets start from MIN.
+  // This helps to avoid complicated conditions below.
+  if (Min == I1->From() && Min == I2->From()) {
----------------
It is not clear what is the complicated condition that you'd like to avoid. Could you please elaborate?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:189-201
+  if (Min == I1->From() && Min == I2->From()) {
+    if (I1->To() > I2->To()) {
+      // The second range is entirely inside the first one. Skip it.
+      // Check for the end of the range for every incrementation.
+      if (++I2 == E2)
+        return AppendRest(I1, E1);
+    } else {
----------------
I'd like to see similar visual comments that we have in `intersect`.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:204-205
+  while (true) {
+    // I1->From() shall be lower than I2->From().
+    // Otherwise, swap the iterators.
+    if (I1->From() > I2->From()) {
----------------
Let's reuse as much as possible from `intersect`, both code and comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:207-208
+    if (I1->From() > I2->From()) {
+      std::swap(I1, I2);
+      std::swap(E1, E2);
+    }
----------------
We could reuse `SwapIterators` from `intersect`. Of course for that we need to make a real function out of the lambda.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:211
+
+    // At this point, the next range surely starts with I1->From().
+    F = &I1->From();
----------------



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:212
+    // At this point, the next range surely starts with I1->From().
+    F = &I1->From();
+
----------------
Let's be consistent with `intersect`. Also, you could introduce the variable here, and it is not needed to declare that at L176.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:214
+
+    // Build a new range.
+    while (true) {
----------------



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:242
+      // Every next range of the first set always go after the second range.
+      // So swap the iterators without any check.
+      std::swap(I1, I2);
----------------



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

https://reviews.llvm.org/D99797