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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to