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