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