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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:297-307
+bool clang::ento::RangeSet::isUnsigned() const {
+  return begin()->From().isUnsigned();
+}
+
+uint32_t clang::ento::RangeSet::getBitWidth() const {
+  return begin()->From().getBitWidth();
+}
----------------
We should add `assert(!isEmpty());` in all three


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:645-676
+    // CastRangeSize is an amount of all possible values of cast type.
+    // Example: `char` has 256 values; `short` has 65536 values.
+    // But in fact we use `amount of values` - 1, because
+    // we can't keep `amount of values of UINT64` inside uint64_t.
+    // E.g. 256 is an amount of all possible values of `char` and we can't keep
+    // it inside `char`.
+    // And it's OK, it's enough to do correct calculations.
----------------
Maybe we can extract this into something like `truncateTo` and the next `if` to `convertTo` private methods?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:117-118
+  template <typename T>
+  static constexpr APSIntType APSIntTy = APSIntType(sizeof(T) * 8,
+                                                    !is_signed_v<T>);
+
----------------
Great, that works too!


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:120
+
+  template <typename T> const llvm::APSInt &from(T X) {
+    static llvm::APSInt Int = APSIntTy<T>.getZeroValue();
----------------
Default to `BaseType`?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:126
 
-  Range from(const RawRange &Init) {
+  template <typename T> Range from(const RawRangeT<T> &Init) {
     return Range(from(Init.first), from(Init.second));
----------------
Default to `BaseType`?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:130
 
-  RangeSet from(const RawRangeSet &Init) {
+  template <typename T>
+  RangeSet from(RawRangeSetT<T> Init, APSIntType Ty = APSIntTy<BaseType>) {
----------------
Default to `BaseType`?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:131
+  template <typename T>
+  RangeSet from(RawRangeSetT<T> Init, APSIntType Ty = APSIntTy<BaseType>) {
     RangeSet RangeSet = F.getEmptySet();
----------------
Unused parameter?


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:721-799
+TYPED_TEST(RangeSetCastToNoopTest, RangeSetCastToNoopTest) {
+  // Just to reduce the verbosity.
+  using F = typename TypeParam::FromType; // From
+  using T = typename TypeParam::ToType;   // To
+
+  using TV = TestValues<T>;
+  constexpr auto MIN = TV::MIN;
----------------
If loop and promotion share the same test case, why should we split them into two groups?


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

https://reviews.llvm.org/D103094