martong added a comment.
I think, the visual comments that we have in `intersect` makes the code of
`intersect` a lot easier to follow. Could you please add similar visual
comments here? Also, I find `First` and `Second` in `intersect` way more better
naming than `I1` and `I2`, could you please update?
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:169
+
+ iterator I1 = LHS.begin();
+ iterator E1 = LHS.end();
----------------
I think, the naming conventions we have in `intersect` are easier to follow.
Could we call `I1` as `First` and `I2` as `Second`?
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:181
+ // Append the rest of the ranges from another range set to the Result
+ // and return the later.
+ auto AppendRest = [&Result](iterator I, iterator E) {
----------------
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:188
+ // Handle a corner case first when both range sets start from MIN.
+ // This helps to avoid complicated conditions below.
+ if (Min == I1->From() && Min == I2->From()) {
----------------
It is not clear what is the complicated condition that you'd like to avoid.
Could you please elaborate?
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:189-201
+ if (Min == I1->From() && Min == I2->From()) {
+ if (I1->To() > I2->To()) {
+ // The second range is entirely inside the first one. Skip it.
+ // Check for the end of the range for every incrementation.
+ if (++I2 == E2)
+ return AppendRest(I1, E1);
+ } else {
----------------
I'd like to see similar visual comments that we have in `intersect`.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:204-205
+ while (true) {
+ // I1->From() shall be lower than I2->From().
+ // Otherwise, swap the iterators.
+ if (I1->From() > I2->From()) {
----------------
Let's reuse as much as possible from `intersect`, both code and comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:207-208
+ if (I1->From() > I2->From()) {
+ std::swap(I1, I2);
+ std::swap(E1, E2);
+ }
----------------
We could reuse `SwapIterators` from `intersect`. Of course for that we need to
make a real function out of the lambda.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:211
+
+ // At this point, the next range surely starts with I1->From().
+ F = &I1->From();
----------------
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:212
+ // At this point, the next range surely starts with I1->From().
+ F = &I1->From();
+
----------------
Let's be consistent with `intersect`. Also, you could introduce the variable
here, and it is not needed to declare that at L176.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:214
+
+ // Build a new range.
+ while (true) {
----------------
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:242
+ // Every next range of the first set always go after the second range.
+ // So swap the iterators without any check.
+ std::swap(I1, I2);
----------------
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
