[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Anna Zaks via Phabricator via cfe-commits]

zaks.anna added a comment.

These are great additions!

Going back to my comment about adding these to CheckerContext, I think we should be adding helper functions as methods on CheckerContext as it is **the primary place where checker writers look for helpers**. Two of the three methods added take CheckerContext as an argument, so what is the reason for adding them elsewhere?

As for the svalComparesTo method, if we want to make it available to the two callbacks that do not have checker context, we can include a method on checker context that calls into a helper in CheckerHelpers.h. However, given that even this patch is not using the function, I would argue for leaving it as a helper function internal to CheckerContext.cpp.



================
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:119
+           << BinaryOperator::getOpcodeStr(B->getOpcode())
+           << "' expression is undefined due to negative value on the right "
+              "side of operand";
----------------
"right side of operand" does not sound right..

How about: 
"The result of the '<<' expression is undefined due to negative value on the right side of operand" 
-> 
"The result of the left shift is undefined because the right operand is negative"
or
"The result of the '<<' expression is undefined because the right operand is negative"


================
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:126
+           << BinaryOperator::getOpcodeStr(B->getOpcode())
+           << "' expression is undefined due to shift count >= width of type";
+      } else {
----------------
It's best not to use ">=" in diagnostic messages.
Suggestions: "due to shift count >= width of type" ->
- "due to shifting by a value larger than the width of type"
- "due to shifting by 5, which is larger than the width of type 'int'" // Providing the exact value and the type would be very useful and this information is readily available to us. Note that the users might not see the type or the value because of macros and such.


================
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:98
+
+bool clang::ento::svalComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+                                 SVal RHSVal, ProgramStateRef State) {
----------------
How about evalComparison as a name for this?


================
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:121
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
----------------
Please, use doxygen comment style here and below.


Repository:
  rL LLVM

https://reviews.llvm.org/D30295