[PATCH] D41938: [Analyzer] SValBuilder Comparison Rearrangement (with Restrictions and Analyzer Option)

[Artem Dergachev via Phabricator via cfe-commits]

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

This looks good with a super tiny nit regarding comments about signed integers. George, are you happy with the changes? (:



================
Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:675-677
+  /// is on the right. This is only done if both concrete integers are greater
+  /// than or equal to the quarter of the minimum value of the type and less
+  /// than or equal to the quarter of the maximum value of that type.
----------------
I believe that we should mention that the integers are signed.


================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:330
+                      nonloc::ConcreteInt(Max), SVB.getConditionType());
+  if (auto DV = IsCappedFromAbove.getAs<DefinedSVal>()) {
+    if (State->assume(*DV, false))
----------------
baloghadamsoftware wrote:
> george.karpenkov wrote:
> > 6 lines of branching is probably better expressed as
> > 
> > ```
> > if (!isa<DefinedSVal>(IsCappedFromAbove) || State->assume(*dyn_cast<DefinedSVal>(IsCappedFromAbove), false))
> >    return false
> > ```
> SVal is not a pointer, so isa<>() and dyn_cast<>() does not work here.
`.getAs<>()` would have been similar, but i'm not fond of double checks either.


================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:429
+  if (BinaryOperator::isComparisonOp(Op)) {
+    // Prefer comparing to a non-negative number.
+    // FIXME: Maybe it'd be better to have consistency in
----------------
george.karpenkov wrote:
> It seems that having a concrete positive RHS is a part of your rewrite rule. In that case, that should be documented in a high-level overview of the rewrite rules.
That's an invariant that we're maintaining in other places as well.


================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:774
 
+      if (Optional<NonLoc> V = tryRearrange(state, op, lhs, rhs, resultTy))
+        return *V;
----------------
george.karpenkov wrote:
> I would expect that checking the analyzer option would be performed here?
> 
Nope, not yet. The flag is only for range-checked rearrangements.


https://reviews.llvm.org/D41938