vsavchenko added a comment.
In D86465#2238614 <https://reviews.llvm.org/D86465#2238614>, @martong wrote:
> These are really promising figures, nice work! (And the measurement stuff
> itself is also a great addition, thanks for that!)
Thanks 😊
> Not that it would matter much, but I was just wondering why there is a
> slightly bigger memory usage in some of the `docker/small` projects? The most
> conspicuous is `oatpp`.
The measurements fluctuate quite significantly and as you can see the means for
both `old` and `new` experiments for `oatpp` are pretty close. So, I would say
that the memory differences are not conclusive.
This being said, the performance measurements should also be taken with a grain
of salt. We can't really say that we get **5-10%** performance improvement for
sure, but that analysis //tends// to be faster instead.
================
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(); }
----------------
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.
================
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) {
----------------
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`
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:272-273
+private:
+ RangeSet(ContainerType *RawContainer) : Impl(RawContainer) {}
+ RangeSet(UnderlyingType Ptr) : Impl(Ptr) {}
----------------
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.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:322-325
+ auto swap = [&First, &FirstEnd, &Second, &SecondEnd]() {
+ std::swap(First, Second);
+ std::swap(FirstEnd, SecondEnd);
+ };
----------------
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.
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
