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

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 4 08:27:41 PDT 2017


NoQ added a comment.

Hello, sorry for your having to wait on me. The implementation looks good, i'm only having a couple of public API concerns.



================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46-49
+bool isExprResultConformsComparisonRule(CheckerContext &C,
+                                        BinaryOperatorKind CompRule,
+                                        const Expr *LHSExpr, const SVal RHSVal);
+bool isGreaterEqual(CheckerContext &C, const Expr *E, unsigned long long Val);
----------------
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.


================
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:101
+    const SVal RHSVal) {
+  ProgramStateRef State = C.getState();
+
----------------
I believe it is enough to pass only `State` to this function. `SValBuilder` and `ConstraintManager` objects can be obtained from the state's `ProgramStateManager`. 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





More information about the cfe-commits mailing list