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

Denys Petrov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 17 05:26:35 PDT 2021


ASDenysPetrov added a comment.

@vsavchenko Repled in inlines. Preparing an update.



================
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);
----------------
vsavchenko wrote:
> That is a bit of a red flag because instantly my intuition tells me that there should be a linear option.
Unfortunately, `castTo` is N^2 because technically we can call `unite`(which is N) inside it N times.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:248
+    RangeSet castTo(RangeSet What, APSIntType Ty);
+    RangeSet castTo(RangeSet What, QualType T);
 
----------------
vsavchenko wrote:
> 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.
+1. Is there any place where we can put tasks as tech debt or just keep noting with FIXME/TODO's?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:692
+  ContainerType Result;
+  ContainerType RHS;
+
----------------
vsavchenko wrote:
> 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.
What about `Tmp`?


================
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.
----------------
vsavchenko wrote:
> 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?
>Maybe you want to talk about the maximum stored value?
Yes. I should rephrase the comment.
E.g.: 128 is an amount of all possible values of `char` and we can't keep it inside `char`.


================
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
----------------
vsavchenko wrote:
> 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.
>we don't need to go and unite them.
Um, yes. 'unite' only needed for truncated range sets which have more then one range. I'll try to refactor this.
That will help make the algorithm //linear //for most cases.


================
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;
----------------
vsavchenko wrote:
> I don't really understand what's going on here.
This is a generalized version of `from` function. But this one you can use independently from which type the current test case uses. You can set a custom type to get a range set for a given type. This allows to get range sets based on different types (e.g. `char` and `int`)  in a single test case.
Previously `from` produced range sets only for the type specified by a 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