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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@martong

> Nice, assiduous work!

Many thanks for your time! Your work is not less assiduous!

> (LHS, RHS) swaps

it doesn't really matter, as the operation is comutative, but, yes, it does matter to depict the particular test case. I'll revise twise LHS-RHS's.

> 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)

Aha. I saw you patch. I'll join to review it.



================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:441
+  //        ___/__/_____\__\___   ___/___________\___
+  this->checkUnite({{MID, MID}}, {A, D}, {{A, D}});
+  this->checkUnite({{B, C}}, {A, D}, {{A, D}});
----------------
martong wrote:
> 
There are three overloads of `checkUnite` which correspond to three overloads of `RangeSet::unite`. I made it consistent to other check-functions (`add` e.g.).


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


================
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}},
----------------
martong wrote:
> I think we could better format these more complex cases.
clang-fromat acts on its own. But I agree, it looks the way better. I'll consider wrapping it into `// clang-format on/off` directives.


================
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:
> martong wrote:
> > 
> What do you think about this format? The result can be easily verified this way I think, but a bit ugly ...
I think ASCII art does this job. Let code look as code :)


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

https://reviews.llvm.org/D99797