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