steakhal added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:149
+
+RangeSet RangeSet::Factory::unite(RangeSet Original, llvm::APSInt Point) {
+ return unite(Original, Range(ValueFactory.getValue(Point)));
----------------
martong wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > Why do you take `APSInt`s by value? Generally, we take them by reference.
> > I want to send a message to the caller that he can pass an arbitrary
> > **APSInt** without warrying about making it permanent (aka stored by the
> > Factory). But we can revise this contract and carry this responsibility to
> > a caller.
> > Why do you take `APSInt`s by value? Generally, we take them by reference.
>
> Actually, it is specific to `BasicValueFactory` to cache the `APSInt`s,
> however, it might not be the best practice. I doubt that somebody has ever
> measured the performance of passing APSInts by value, so my guess is the
> caching of `APSInt`s might be an early optimization that might be more
> harmful than advantageous. On top of all this, we do the caching
> inconsistently, just consider the member functions of `APSIntType`, they all
> return by value.
>
> Perhaps (totally independently from this patch of course), it might be worth
> to have a measurement/comparison with removed cache and pass by value.
>
Okay, then we shall leave this as-is now.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:81
const llvm::APSInt &from(BaseType X) {
- llvm::APSInt Dummy = Base;
- Dummy = X;
- return BVF.getValue(Dummy);
+ static llvm::APSInt Base{sizeof(BaseType) * 8,
+ std::is_unsigned<BaseType>::value};
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > Shouldn't you use `sizeof(BaseType) * CHAR_BIT` instead?
> Agree. It's better to avoid magic numbers. I'll fix.
It's not only that but just imagine testing a clang on a special hardware where
they have let's say 9 bit bytes, for parity or something similar stuff.
The test would suddenly break.
Although I suspect there would be many more things to break TBH xD
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99797/new/
https://reviews.llvm.org/D99797
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
