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

Valeriy Savchenko via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 17 04:11:52 PDT 2021


vsavchenko added a comment.

OK, about the quadratic algorithm.  I can think of one solution that is actually linear asymptotically, but because of the constant factor can be worse in practice (we usually have extremely small range sets).  So, while I don't like it, it's probably fine.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:245-246
+    ///
+    /// Complexity: O(N^2)
+    ///             where N = size(What)
+    RangeSet castTo(RangeSet What, APSIntType Ty);
----------------
That is a bit of a red flag because instantly my intuition tells me that there should be a linear option.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:248
+    RangeSet castTo(RangeSet What, APSIntType Ty);
+    RangeSet castTo(RangeSet What, QualType T);
 
----------------
If we talk about types for ranges, the interface we have is incomplete.
We should introduce something like `getType` for range sets, so that the user can have a more holistic picture about types and ranges.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:692
+  ContainerType Result;
+  ContainerType RHS;
+
----------------
We have LHS (left-hand side) and RHS (right-hand side) as a common convention for naming operands of binary operators.
The fact that you later on use it as an RHS for `unite`, doesn't make it a good name.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:697
+  // But in fact we use `number of values` - 1, because
+  // we can't hold `UINT64 number of values` inside uint64_t.
+  // And it's OK, it's enough to do correct calculations.
----------------
That's incorrect. `uint64_t` holds **exactly** this many values.  If you count every number from `0` to `UINT64_MAX`, you get `UINT64_MAX + 1`, which is the number you are talking about.
Maybe you want to talk about the maximum stored value?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:701-730
+  // Convert each range from the original range set.
+  for (const Range &R : What) {
+    // Get bounds of the next range.
+    APSInt FromInt = R.From();
+    APSInt ToInt = R.To();
+    RHS.clear();
+    // CurrentRangeSize is a number of all possible values of the current range
----------------
I think we should have a shortcut for widening casts, where we don't need to go and unite them.  We just literally need to convert every range from one type to another and be done with it.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:129-141
+  template <typename T> RangeSet from(APSIntType Ty, RawRangeSetT<T> Init) {
+    llvm::APSInt First, Second;
+    Ty.apply(First);
+    Ty.apply(Second);
     RangeSet RangeSet = F.getEmptySet();
     for (const auto &Raw : Init) {
+      First = Raw.first;
----------------
I don't really understand what's going on here.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:646-647
+TYPED_TEST(RangeSetTest, RangeSetCastToTest) {
+  // TODO: Simplify calls below using list of types (int8_t, int16_t,
+  // int32_t, etc.).
+
----------------
Yes, please.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:695
+
+template <typename BaseType> void RangeSetTest<BaseType>::checkCastTo_NOOP() {
+  // Just to reduce the verbosity.
----------------
Test cases don't belong in `RangeSetTest`.

Each piece of code like this should be a separate test case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103094/new/

https://reviews.llvm.org/D103094



More information about the cfe-commits mailing list