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

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.

Nice, assiduous work! The tests are awesome!
LGTM, with minor revisions. Please check out my suggestions about the tests' formatting and there are those disturbing (LHS, RHS) swaps in the comments.

I am going to continue with the next patch in the stack. I recognize that the handling of casts is very important and problems related to not handling them occurs even more frequently as other parts of the engine evolve (e.g. https://reviews.llvm.org/D113753#3167134)



================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:441
+  //        ___/__/_____\__\___   ___/___________\___
+  this->checkUnite({{MID, MID}}, {A, D}, {{A, D}});
+  this->checkUnite({{B, C}}, {A, D}, {{A, D}});
----------------



================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:449
+  //        ___/___________\___   ___/___________\___
+  this->checkUnite({{MIN, MIN}}, MIN, {{MIN, MIN}});
+  this->checkUnite({{A, B}}, {A, B}, {{A, B}});
----------------
I think, either we should use `{{X, X}}` or `X` everywhere, but not mixed. 


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:525-526
+
+  // RHS =>
+  // LHS =>   /\   /\            =   __   __
+  //        _/__\_/__\_/\_/\_/\_   _/__\_/__\_/\_/\_/\_
----------------
LHS and RHS is swapped here?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:528-529
+  //        _/__\_/__\_/\_/\_/\_   _/__\_/__\_/\_/\_/\_
+  this->checkUnite({{MIN, A}, {A + 2, B}}, {{MID, C}, {C + 2, D - 2}, {D, MAX}},
+                   {{MIN, A}, {A + 2, B}, {MID, C}, {C + 2, D - 2}, {D, MAX}});
+  this->checkUnite({{MIN, MIN}, {A, A}}, {{B, B}, {C, C}, {MAX, MAX}},
----------------
I think we could better format these more complex cases.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:533-534
+
+  // RHS =>
+  // LHS =>             /\   /\   =            __   __
+  //        _/\_/\_/\__/__\_/__\_   _/\_/\_/\_/__\_/__\_
----------------
LHS and RHS is swapped?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:536-537
+  //        _/\_/\_/\__/__\_/__\_   _/\_/\_/\_/__\_/__\_
+  this->checkUnite({{C + 2, D - 2}, {D, MAX}}, {{MIN, A}, {A + 2, B}, {MID, C}},
+                   {{MIN, A}, {A + 2, B}, {MID, C}, {C + 2, D - 2}, {D, MAX}});
+  this->checkUnite({{C, C}, {MAX, MAX}}, {{MIN, MIN}, {A, A}, {B, B}},
----------------



================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:541-542
+
+  // RHS =>
+  // LHS =>   _   /\   _   /\   _   /\  =
+  //        _/_\_/__\_/_\_/__\_/_\_/__\_
----------------
LHS and RHS is swapped?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:558-559
+
+  // RHS =>
+  // LHS =>   /\   _   /\   _   /\   _  =
+  //        _/__\_/_\_/__\_/_\_/__\_/_\_
----------------
LHS and RHS is swapped here as well.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:582
+  this->checkUnite({{A, A}, {B, MID}, {D, D}},
+                   {{A, A}, {B, B}, {MID + 1, D - 1}}, {{A, A}, {B, D}});
+
----------------



================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:590-592
+  this->checkUnite({{A, B - 1}, {B + 1, C - 1}, {C + 2, D}, {MAX - 1, MAX}},
+                   {{MIN, MIN}, {B, MID}, {MID + 1, C}, {C + 4, D - 1}},
+                   {{MIN, MIN}, {A, C}, {C + 2, D}, {MAX - 1, MAX}});
----------------



================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:590-592
+  this->checkUnite({{A, B - 1}, {B + 1, C - 1}, {C + 2, D}, {MAX - 1, MAX}},
+                   {{MIN, MIN}, {B, MID}, {MID + 1, C}, {C + 4, D - 1}},
+                   {{MIN, MIN}, {A, C}, {C + 2, D}, {MAX - 1, MAX}});
----------------
martong wrote:
> 
What do you think about this format? The result can be easily verified this way I think, but a bit ugly ...


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

https://reviews.llvm.org/D99797