[PATCH] D41816: [analyzer] Model and check unrepresentable left shifts

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

The diagnostic text looks to me, but I do have a comment about the nested 'if' inline.



================
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:150
+            SB.getKnownValue(state, C.getSVal(B->getRHS()));
+        if ((unsigned) RHS->getZExtValue() > LHS->countLeadingZeros()) {
+          OS << "The result of the left shift is undefined due to shifting \'"
----------------
This inner 'if' looks fishy to me because if the 'else' branch is ever taken then OS will be empty.

If the else branch can't be taken then you should turn it into an assert(). If it can be taken, then you should make sure that the fall through goes through the "default" else case at the bottom. One way to do this is to pull out the "is representable logic" into a helper function and call that in the containing 'else if'.

Something like:

```
if (B->getOpcode() == BinaryOperatorKind::BO_Shl && isLeftShiftResultRepresentable(LHS, RHS)) {
  OS << "The result of the left shift ..."
}
```


https://reviews.llvm.org/D41816