ASDenysPetrov added a comment.
I didn't check correctness of each Factory function but if it passes all tests
and improves performance, I think it's enough.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:77-78
+ //
+ // * Range sets are usually very simple, 1 or 2 ranges.
+ // That's why llvm::ImmutableSet is not perfect.
+ //
----------------
That's very usefull description for reviewers, but I think is redundant for
newcomers, when you're reading this you can't understand why it compares to
`ImmutableSet` at all. I think this preamble only relates to the Summary of
this patch as the core reason of this change.
You can just mention the fact that formerly it was an `ImmutableSet`. See
//http// for details.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:133
+ /// where N = size(Original)
+ RangeSet add(RangeSet Original, Range Element);
+
----------------
We can add one more instance of `add` (and maybe others): `RangeSet
add(RangeSet original, llvm::APSInt &point);`.
Since we have `RangeSet(Factory &F, const llvm::APSInt &Point)` ctor, why
wouldn't we have similar functionality in the `Factory`?
It could simplify function `RangeSetTest::from` in tests and be useful across
the code on the whole.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:178
+ /// where N = size(From)
+ RangeSet deletePoint(const llvm::APSInt &Point, RangeSet From);
+ /// Negate the given range set.
----------------
IMHO you should change params with each other:
`deletePoint(const llvm::APSInt &Point, RangeSet From)` ->
`deletePoint(RangeSet Original, const llvm::APSInt &Point)`
This will look more consistent with other interfaces like in `add` or
`intersect` functions.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:105
+ void checkNegate(RawRangeSet RawOriginal, RawRangeSet RawExpected) {
+ wrap(&Self::checkNegateImpl, RawOriginal, RawExpected);
+ }
----------------
Explain, please, what's the matter to use `wrap` everywhere, not just call
`checkNegateImpl` explicitly?
Won't the call works and looks the same? Like:
```
this->checkNegateImpl({{MIN, A}}, {{MIN, MIN}, {D, MAX}});
```
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:185
+
+using IntTypes = ::testing::Types<int8_t, uint8_t, int16_t, uint16_t, int32_t,
+ uint32_t, int64_t, uint64_t>;
----------------
Thank you for making it in a proper way, instead of a list with different type
instantiations.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86465/new/
https://reviews.llvm.org/D86465
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
