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