[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 6 inline comments as done.
donat.nagy added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:94-98
+void BitwiseShiftValidator::run() {
+  if (isValidShift()) {
+    Ctx.addTransition(State, createNoteTag());
+  }
+}
----------------
donat.nagy wrote:
> NoQ wrote:
> > Because `addTransition()` is a very confusing API that's very easy to misuse in subtle ways (causing unexpected state splits), I feel anxious when I see functions like
> > ```
> >   bool isValidShift();
> >   bool isOvershift();
> >   bool isOperandNegative(OperandSide Side);
> >   bool isLeftShiftOverflow();
> > ```
> > that look like boolean getters but in fact call `addTransition()` under the hood. If we could at least call them `checkOvershift()` etc., that'd be much better. Ideally I wish there was some safeguard against producing redundant transitions, like make them return an `ExplodedNode` and chain them, or something like that.
> These functions were originally called `checkXXX()` but I felt that the meaning of their boolean return value was too confusing with that naming scheme.
> 
> I'll try to ensure that `emitReport` or `addTransition` are only called on the topmost layer (the `run()` method) and change the return types of internal subroutines to make them return nullable pointers (e.g. `std::unique_ptr<PathSensitiveBugReport>`) instead of `bool`s. This would (1) eliminate the "what does `true` mean at this point?" issues (2) clarify that this checker never performs more than one transition. (If the shift is invaild, we transition to a sink node by emitting a bug; if the shift is vaild, we perform a transition to record the state update.)
I eliminated `isValidShift()` by merging it with `run()` (which was very short) and renamed the rest of the functions to `checkXXX`.

Now the exploded graph is only modified in the top-level method `run()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:142
+                                              BinaryOperator::Opcode Comparison,
+                                              long long Limit) {
+  SValBuilder &SVB = Ctx.getSValBuilder();
----------------
donat.nagy wrote:
> steakhal wrote:
> > One does not frequently see the `long long` type.
> > Do you have a specific reason why `int` or `unsigned` wouldn't be good?
> I inherited this type choice from old code and didn't think much about it. My guess is that `LimitVal` needs to be a signed integer (because e.g. `-1 < 0u` would be evaluated to false) and it's a `long long` because that's guaranteed to represent all `unsigned` values. (However, the actual values of `Limit` are very small.)
> 
> I'll probably use `int` for `Limit` and `LimitVal` with some comments to clarify the reason for this type choice.
I changed the type of the `Limit` argument to `unsigned` (because that's what this function is called with) and changed the type of `LimitVal` to `IntTy` with a comment that describes that it must be signed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312