martong added a comment.
Thanks for you patience Denys! Finally I had some time for the review. Nice
work!
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:242
+ ///
+ /// This function optimized for each of the six cast cases:
+ /// - noop
----------------
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:358
+ assert(!isEmpty());
+ return begin()->From().isUnsigned();
+}
----------------
Probably it is unrelated to this patch, but
Could it happen that `(++begin())->From().isUnsigned()` gives a different
signedness? Or we had a separate assertion when we added the second `Range`?
The same question applies to the below two functions as well. Seems like in
`unite` we don't have such validity check...
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:687
+
+ if (IsConversion && (!IsPromotion || !What.isUnsigned()))
+ return makePersistent(convertTo(What, Ty));
----------------
Could you please explain why do we need the `|| !What.isUnsigned()` part of the
condition?
This would make much more sense to me:
```
if (IsConversion && !IsPromotion)
return makePersistent(convertTo(What, Ty));
```
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:690
+
+ // Promotion from unsigneds to signeds/unsigneds left.
+
----------------
martong wrote:
> I think it would be more consistent with the other operations (truncate and
> convert) if we had this logic in its own separate function: `promoteTo`. And
> this function should be called only if `isPromotion` is set (?)
This comment is confusing, since we can get here also if
`isConversion` is false and
`isPromotion` is false
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:690-711
+ // Promotion from unsigneds to signeds/unsigneds left.
+
+ using llvm::APInt;
+ using llvm::APSInt;
+ ContainerType Result;
+ // We definitely know the size of the result set.
+ Result.reserve(What.size());
----------------
I think it would be more consistent with the other operations (truncate and
convert) if we had this logic in its own separate function: `promoteTo`. And
this function should be called only if `isPromotion` is set (?)
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:740-741
+ uint64_t CurrentRangeSize = (ToInt - FromInt).getZExtValue();
+ // This is an optimization for specific case when we are enough to cover
+ // the whole range.
+ Dummy.clear();
----------------
?
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:773
+ ContainerType Result;
+ ContainerType Dummy;
+ // Divide the cast into two phases (presented as loops here).
----------------
We could certainly have a better name for this. E.g. `SecondHalf`. (And
`FirstHalf` instead of `Result`?)
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:774-791
+ // Divide the cast into two phases (presented as loops here).
+ // First phase(loop) works when casted values go in ascending order.
+ // E.g. char{1,3,5,127} -> uint{1,3,5,127}
+ // Interrupt the first phase and go to second one when casted values start
+ // go in descending order. That means that we crossed over the middle of
+ // the type value set (aka 0 for signeds and MAX/2+1 for unsigneds).
+ // For instance:
----------------
I think this comment would be better positioned as a header comment for the
entire function.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:792
+ // unite -> uchar(-2, 1)
+ auto CastRange = [Ty, &VF = ValueFactory](const Range &R) -> Bounds {
+ // Get bounds of the given range.
----------------
Why not `-> Range`?
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:802
+ // Phase 1. Fill the first array.
+ APSInt LastFromInt = Ty.getMinValue();
+ const auto *It = What.begin();
----------------
Would it be a better name:`LastConvertedInt`? The naming `From` is confusing
because that suggests the value we cast from, however, this variable refers to
the casted to values, isn't it?
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:812-814
+ if (NewBounds.first > NewBounds.second) {
+ Dummy.emplace_back(ValueFactory.getMinValue(Ty), NewBounds.second);
+ Result.emplace_back(NewBounds.first, ValueFactory.getMaxValue(Ty));
----------------
I think this case (wrap) deserves a comment.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:728
+
+ using TV = TestValues<T>;
+ constexpr auto MIN = TV::MIN;
----------------
It doesn't matter, becasue `T` and `F` are equal here, but for consistency with
the other tests.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:827
+ if (is_signed_v<F>) {
+ this->template checkCastTo<F, T>({{MIN, MIN}, {MAX, MAX}}, {{MAX, MIN}});
+ this->template checkCastTo<F, T>({{MID, MID}, {MAX, MAX}}, {{MAX, MID}});
----------------
I am getting lost. Why don't you check against `ToMIN` and `ToMAX` here? Could
you explain e.g. with int16->int8?
It is confusing that at many places you test against `MIN`, `MAX`, `A`, ... and
the conversion is happening automatically by the `checkCastTo` template. Would
it be more explicit to use everywhere `ToMIN`, `ToA`, `ToB`, ... and check
against them?
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:874-879
+ constexpr auto FromA = TV::FromA;
+ constexpr auto ToA = TV::ToA;
+ constexpr auto FromB = TV::FromB;
+ constexpr auto ToB = TV::ToB;
+ this->template checkCastTo<F, T>({{FromA, ToA}, {FromB, ToB}},
+ {{FromA, ToA}});
----------------
Could you please elaborate what do we test here?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103094/new/
https://reviews.llvm.org/D103094
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
