vsavchenko marked 3 inline comments as done.
vsavchenko added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:77
+ assert(!isEmpty());
+ // NOTE: It's a shame that we can't implement 'getMaxValue' without scanning
+ // the whole tree to get to the last element.
----------------
xazax.hun wrote:
> Yeah, this is quite unfortunate. But you might end up calling this a lot for
> bitwise operations. I wonder if it is worth to solve this problem before
> commiting this patch. I do not insist though.
I was even thinking about fixing it myself, this shouldn't be very hard, Also
addition of `lower_bound` and `upper_bound` can really help implementing
`Includes` for `RangeSet`.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:388
+ /// it will return the range [x_0, y_N].
+ static Range roughen(RangeSet Origin) {
+ assert(!Origin.isEmpty());
----------------
xazax.hun wrote:
> Is `roughen` part of a nomenclature? If not, what about something like
> `cover`?
Yeah 😆 I spend quite some time thinking about a proper name for that one. It is
used to be `coarsen` (yep, yuck!), but I don't really feel "cover" either. Mb
something like "unify" will work? What do you think?
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:418
+
+ auto ConvertedCoarseLHS = convert(CoarseLHS, ResultType);
+ auto ConvertedCoarseRHS = convert(CoarseRHS, ResultType);
----------------
xazax.hun wrote:
> Why do we need this conversion?
> Do we want to model a cast in the code somewhere?
> If so, I think this is more like a workaround and in the future we would need
> an explicit way to represent those cast operations. It might be worth to have
> a TODO.
Those ranges that we inferred have nothing to do with upper operations. I
mean, in theory, `VisitBinaryOperator` can take care of conversions, but I
don't know how it can be done in the most generic way. In this particular
situation, we really care that `From` and `To` don't change signs or something
like that after conversion, but for other operators it might be not so
important.
Answering the question of why we even need those, putting it simple - there is
no other way, this is how `APSInt` works, couldn't do anything with values of
different types (e.g. comparison).
I am totally on board with you on the inconvenience of the whole thing. Every
implementer of the `Visit*` method should take care of conversions on their
own. I would say that `SValBuilder` adding special symbolic values for this
kind of casts, can be a solution.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79336/new/
https://reviews.llvm.org/D79336
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
