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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits