NoQ added a comment.
If the numbers are confirmed to be as good as what i've sneak-peeked so far,
that should be pretty amazing. Also whoa, test coverage!~
WDYT about moving introducing a generic "`SmallImmutableSet`" in `llvm/ADT` to
back your implementation?
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:88
+ // structure is preferred.
+ using ImplType = llvm::SmallVector<Range, 4>;
+
----------------
vsavchenko wrote:
> grandinj wrote:
> > Just curious - if they mostly contain 1 or 2 elements, why is N == 4 here?
> Essentially, the main factor is **intuition** 😆
> The main idea here is that allocations are the single most expensive thing
> about ranges, so I want to avoid extra allocations not for 60% of range sets,
> but for 90%. I still should definitely measure performance difference for
> different values of N and gather some stats about the lengths.
Given that you pass those by pointers, i suspect you don't need to fix the size
at all. In fact the small-size of the small vector is, by design, a //runtime//
value and you can use `llvm::SmallVectorImpl *` instead, which can point to
`SmallVector` of any small-size. This would allow you to allocate small vectors
of variable length which you can take advantage of whenever you know the length
in advance.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:135
+ /// @{
+ RangeSet getSet(Range Origin);
+ RangeSet getSet(const llvm::APSInt &From, const llvm::APSInt &To) {
----------------
"But what about `setGet()`???" - some user, probably :)
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:119
+
+ return makePersistent(std::move(Result));
+}
----------------
Given that we're certain from the start that the container will be persistent,
can we save a copy by directly asking the factory to allocate a fresh container?
Also this seems to be one of the cases where variable-sized small vectors would
make sense.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:281
+ //
+ // Shortcut: check that there is even a possibility of the intersection
+ // by checking the two following situations:
----------------
Thanks a lot for adding those comments. These methods were confusing as heck.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:206
+
+ this->checkNegate({{MIN, A}}, {{MIN, MIN}, {D, MAX}});
+ this->checkNegate({{MIN, C}}, {{MIN, MIN}, {B, MAX}});
----------------
Why is it necessary to specify `this->` here?
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
