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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
