steakhal added a comment.
Previously, I liked this. Now I love it!
The benchmarks look promising as well.
I think I can not convince you about not modifying iterators using `++`,`--`,
etc. outside of //loop-expressions//. So be it :D
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:110
+public:
+ using iterator = ImplType::const_iterator;
+
----------------
Shouldn't we call this `const_iterator`?
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:209
+ private:
+ /// Return a persistent version of the given container.
+ RangeSet makePersistent(ContainerType &&From);
----------------
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:211
+ RangeSet makePersistent(ContainerType &&From);
+ /// Construct a new persistent version of the given container.
+ ContainerType *construct(ContainerType &&From);
----------------
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:278
+private:
+ /* implicit */ RangeSet(ContainerType *RawContainer) : Impl(RawContainer) {}
+ /* implicit */ RangeSet(UnderlyingType Ptr) : Impl(Ptr) {}
----------------
Btw in C++20 we will have a way expressing this: `explicit(false)`
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:281-290
+ /// Pin given points to the type represented by the current range set.
+ ///
+ /// This makes parameter points to be in-out parameters.
+ /// In order to maintain consistent types across all of the ranges in the set
+ /// and to keep all the operations to compare ONLY points of the same type,
we
+ /// need to pin every point before any operation.
+ ///
----------------
Aww, excellent. Thanks.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:51
}
-};
+ void print(raw_ostream &OS) const;
----------------
steakhal wrote:
> We have `dump` methods all over the place. Except this.
> I propose to rename this to `dump`.
How about marking this `LLVM_DUMP_METHOD`?
Also, defining the `operator<<` would come handly later.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:53-55
+ // In order to keep non-overlapping ranges sorted, we can compare only From
+ // points.
+ inline bool operator<(const Range &RHS) const { return From() < RHS.From(); }
----------------
vsavchenko wrote:
> steakhal wrote:
> > It's a good practice to define comparison operators as //friend// functions
> > inline.
> > Even if we don't rely on implicit conversions.
> It doesn't seem like there is a single opinion on this in the codebase.
Okay.
================
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) {
----------------
vsavchenko wrote:
> steakhal wrote:
> > NoQ wrote:
> > > "But what about `setGet()`???" - some user, probably :)
> > Why don't we call this `createSetOf`?
> > And `createEmptySet`.
> > I know that we don't create the empty set, but then what does a //factory//
> > if not create stuff?
> The naming is this way to be consistent with `ImmutableSet::Factory`
Makes sense.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:272-273
+private:
+ RangeSet(ContainerType *RawContainer) : Impl(RawContainer) {}
+ RangeSet(UnderlyingType Ptr) : Impl(Ptr) {}
----------------
vsavchenko wrote:
> steakhal wrote:
> > vsavchenko wrote:
> > > steakhal wrote:
> > > > Missing `explicit`.
> > > More like missing `/* implicit */` because it is intentional
> > It doesn't have a too long identifier.
> > The users can always refer to it `auto R = RangeSet(...)`, so we still
> > don't repeat ourselves.
> > Do you have any particular counterexample?
> > Probably the tests will become slightly more bloated but eh. whatever.
> These constructors are `private` and used in `Factory` methods. IMO they
> make those methods less cluttered.
Okay.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:326
+
+ // If we ran out of ranges in one set, but not the other,
+ // it means that those elements are definitely not in the
----------------
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:462-470
// Negate all other ranges.
- for (iterator e = end(); i != e; ++i) {
+ for (; It != End; ++It) {
// Negate int values.
- const llvm::APSInt &newFrom = BV.getValue(-i->To());
- const llvm::APSInt &newTo = BV.getValue(-i->From());
- // Add a negated range.
- newRanges = F.add(newRanges, Range(newFrom, newTo));
- }
-
- if (newRanges.isSingleton())
- return newRanges;
+ const llvm::APSInt &NewFrom = ValueFactory.getValue(-It->To());
+ const llvm::APSInt &NewTo = ValueFactory.getValue(-It->From());
+ // Add a negated range.
----------------
It might be overkill, but we can do this using transform.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:495-507
+void RangeSet::dump(raw_ostream &OS) const {
bool isFirst = true;
- os << "{ ";
- for (iterator i = begin(), e = end(); i != e; ++i) {
+ OS << "{ ";
+ for (const Range &R : *this) {
if (isFirst)
isFirst = false;
else
----------------
Hmm, we could simplify this further, assuming `Range::operator<<` is defined.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:322-325
+ auto swap = [&First, &FirstEnd, &Second, &SecondEnd]() {
+ std::swap(First, Second);
+ std::swap(FirstEnd, SecondEnd);
+ };
----------------
vsavchenko wrote:
> steakhal wrote:
> > vsavchenko wrote:
> > > steakhal wrote:
> > > > It could definitely bear a longer name.
> > > I think that there is nothing wrong in spelling out `std::swap` twice.
> > > And do we have a naming convention for lambdas, is it a variable or a
> > > function from a naming perspective?
> > That's a good question :)
> > I would say that its a variable, since you can mark it `const`, otherwise
> > you could overwrite it. xD But that's a different story I think.
> > About the `swap` thingie, its a good practice to respect ADL for functions
> > which know to be used via ADL.
> > Even if we don't depend on ADL in this particular case.
> I'm sorry but I still don't get the importance of dropping `std::` here.
Since we don't rely on Argument Dependent Lookup in this particular case, both
versions do the same.
So **it's perfectly fine as-is**.
You should interpret my previous comment in a broader sense, like a guideline.
Which is meaningful mostly in //generic// programming.
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
