a.sidorin added a comment. Wow, thank you for such a contribution! Did you evaluate this checker on some real code?
I have some minor inline comments below. ================ Comment at: lib/StaticAnalyzer/Checkers/CMakeLists.txt:41 IdenticalExprChecker.cpp + RecursionChecker.cpp IvarInvalidationChecker.cpp ---------------- Please fix indentation here ================ Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:12 +// This defines TestAfterDivZeroChecker, a builtin check that performs checks +// for division by zero where the division occurs before comparison with zero. +// ---------------- This description is for other checker, could you update it? ================ Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:21 + +REGISTER_SET_WITH_PROGRAMSTATE(DirtyStackFrames, const clang::StackFrameContext *) + ---------------- The idea of "dirty stack frames" deserves some explanation. As I understand, it describes function calls where some values may change unexpectedly and we cannot consider this calls later. Am I understanding correctly? ================ Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:49 + void checkPostCall(const CallEvent &Call, + CheckerContext &C) const; +}; ---------------- Incorrect indentation. ================ Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:68 + + if (ParentLC->getKind() != LocationContext::StackFrame) + continue; ---------------- We may just use `getParent()->getCurrentStackFrame()` in the loop increment to make the code cleaner. What do you think? ================ Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:79 + const FunctionDecl *PrevFuncDecl = + (const FunctionDecl *)PrevStackFrameCtx->getDecl(); + PrevFuncDecl = PrevFuncDecl->getCanonicalDecl(); ---------------- We usually prefer LLVM `[dyn_]cast<>` where possible. ================ Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:89 + SVal PrevArg = State->getArgSVal(PrevStackFrameCtx, i); + SameArgs = SameArgs && compareArgs(C, State, CurArg, PrevArg); + } ---------------- Maybe we can use early return here? ================ Comment at: test/Analysis/recursion.cpp:27 + +void f2(); +void f3(); ---------------- I'd like to see some more informative function names: for me, it is hard to distinguish between f1-7 here :) It is also hard to determine the function where test starts. https://reviews.llvm.org/D26589 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
