[PATCH] D103094: [analyzer] Implemented RangeSet::Factory::castTo function to perform promotions, truncations and conversions

[Gabor Marton via Phabricator via cfe-commits]

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