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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Math looks good to me :)



================
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());
----------------
vsavchenko wrote:
> 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?
Well, this is basically a one-dimensional //convex hull//...

But i do think that something like `fillGaps()` would be easier to understand.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:418
+
+    auto ConvertedCoarseLHS = convert(CoarseLHS, ResultType);
+    auto ConvertedCoarseRHS = convert(CoarseRHS, ResultType);
----------------
vsavchenko wrote:
> 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.
Yeah i think this is correct. This is exactly how our symbolic expressions aren't type-safe: expression of type T corresponds to result of operations during which operand symbols were promoted to T.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79336/new/

https://reviews.llvm.org/D79336