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

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki marked 4 inline comments as done.
danielmarjamaki added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46
 
-} // end GR namespace
+bool isExprResultConformsComparisonRule(CheckerContext &C,
+                                        BinaryOperatorKind CompRule,
----------------
zaks.anna wrote:
> NoQ wrote:
> > Because you are making these functions public, i think it's worth it to make it obvious what they do without looking at the particular checker code. Comments are definitely worth it. I think function names could be worded better; i suggest `exprComparesTo(const Expr *LHSExpr, BinaryOperatorKind ComparisonOp, SVal RHSVal, CheckerContext &C);` and `isGreaterOrEqual(...)`; i'm personally fond of having CheckerContext at the back because that's where we have it in checker callbacks, but that's not important.
> + 1 on Artem's comment of making the names more clear and providing documentation. Also, should these be part of CheckerContext?
> Also, should these be part of CheckerContext?

I chose not to. Then as NoQ suggested it's not possible to use this when CheckerContext is not available:

"The good thing about requiring only State is that we'd be able to use these functions in checker callbacks that don't provide the CheckerContext object, eg. checkEndAnalysis or checkRegionChanges."


Repository:
  rL LLVM

https://reviews.llvm.org/D30295