vsavchenko added a comment.
I think this iteration is much better, it requires way more description as it
has now. You didn't actually describe anywhere how this algorithm actually
works.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:200-201
+ }
+
+ // Build a new range.
+ while (true) {
----------------
At this point, we know for a fact that the next range we are going to add to
our result set starts with `I1->From()`. No need to check `F` for null or
anything
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:210
+
+ // Check if the second range intersects or adjucent to the first one.
+ if (I1->To() < I2->From() - One)
----------------
nit: "**is** adj**a**cent"
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:212
+ if (I1->To() < I2->From() - One)
+ break;
+
----------------
Why `break`? At this point we know that what we peaked into is actually a part
of another range (in terms of the end result).
And this range is over, you just need to add it to the final result!
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:214-218
+ // We use `F` as a flag to notify that we are in a building of a new
+ // range. Set `From` of a new range if it is not set yet. If it has
+ // already been set, then we are inside this range and just looking for
+ // its end.
+ if (!F)
----------------
You actually don't need it, the moment you swap two iterators and find out that
`I1->From() <= I2->From()`, that's it `I1->From()` is the beginning of the new
range. No need to carry extra flag here.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:227
+ // Now we can surely swap the iterators without any check, as we know
+ // I2-From() to be lower than I1-From().
+ std::swap(I1, I2);
----------------
nit
Additionally, you didn't tell your readers WHY you are even swapping iterators
in the first place and what relationship they have. You need to communicate in
terms of invariants.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:244
+ if (++I1 == E1)
+ goto end1;
+ };
----------------
This goto is always finishing with a return, so we can refactor `goto end1`
into something like `return copyIntoResult(I1, E1);` and `goto end2` into
`return copyIntoResult(I2, E2);`.
As I mentioned before, optional `F` should be removed. On every path we should
know deterministically what is the beginning and what is the end of the range
that we are trying to add.
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
