[PATCH] D79336: [analyzer] Generalize bitwise OR rules for ranges

[Valeriy Savchenko via Phabricator via cfe-commits]

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