> On 2015-Jul-23, at 08:57, Vedant Kumar <v...@apple.com> wrote:
>
> Ok! The NFC preparatory patch is in part1, and the behavior-changing work is
> in part2.
>
> No problems with check-clang.
>
> <checkForFunctionCall-part1.patch><checkForFunctionCall-part2.patch>
Almost there. A bunch more comments on the first patch, but some can
be left for follow-ups. If you fix the function name and flatten the
`MCE` block I'll commit for you.
> diff --git a/lib/Sema/AnalysisBasedWarnings.cpp
> b/lib/Sema/AnalysisBasedWarnings.cpp
> index f2ff48a..ff21076 100644
> --- a/lib/Sema/AnalysisBasedWarnings.cpp
> +++ b/lib/Sema/AnalysisBasedWarnings.cpp
> @@ -171,6 +171,48 @@ enum RecursiveState {
> FoundPathWithNoRecursiveCall
> };
>
> +static RecursiveState nextRecursiveStateInPath(const FunctionDecl *FD,
LLVM coding style prescribes verbs for function names.
Maybe, `getNextRecursiveStateInPath()`? `getNextRecursiveState()`?
> + CFGBlock &Block,
> + RecursiveState State) {
> + if (State != FoundPathWithNoRecursiveCall)
> + return State;
Can you just check this before calling the function? If so, you don't
need to pass `State` in at all.
Actually, this whole function looks like it's just checking for a
recursive path. You could change it to return `bool`, and rename it
something like `hasRecursiveCall()`, and leave the state-related logic
out of the helper. Maybe better in a follow-up patch?
> +
> + // If the current state is FoundPathWithNoRecursiveCall, the successors
> + // will be either FoundPathWithNoRecursiveCall or FoundPath. To determine
> + // which, process all the Stmt's in this block to find any recursive calls.
> + for (const auto &B : Block) {
> + if (B.getKind() != CFGElement::Statement)
> + continue;
> +
> + const CallExpr *CE = dyn_cast<CallExpr>(B.getAs<CFGStmt>()->getStmt());
> + if (!CE || !CE->getCalleeDecl() ||
> + CE->getCalleeDecl()->getCanonicalDecl() != FD)
> + continue;
> +
> + // Skip function calls which are qualified with a templated class.
> + if (const DeclRefExpr *DRE =
> + dyn_cast<DeclRefExpr>(CE->getCallee()->IgnoreParenImpCasts())) {
> + if (NestedNameSpecifier *NNS = DRE->getQualifier()) {
> + if (NNS->getKind() == NestedNameSpecifier::TypeSpec &&
> + isa<TemplateSpecializationType>(NNS->getAsType())) {
> + continue;
> + }
> + }
> + }
> +
> + if (const CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE)) {
> + if (isa<CXXThisExpr>(MCE->getImplicitObjectArgument()) ||
> + !MCE->getMethodDecl()->isVirtual()) {
> + return FoundPath;
> + }
> + } else {
> + return FoundPath;
> + }
I think this would be clearer like this:
const CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE);
if (!MCE || isa<CXXThisExpr>(MCE->getImplicitObjectArgument()) ||
!MCE->getMethodDecl()->isVirtual())
return FoundPath;
> + }
>
> + return State;
This is always `FoundPathWithNoRecursiveCall`, right?
> +}
> +
> static void checkForFunctionCall(Sema &S, const FunctionDecl *FD,
> CFGBlock &Block, unsigned ExitID,
> llvm::SmallVectorImpl<RecursiveState>
> &States,
> @@ -187,42 +229,7 @@ static void checkForFunctionCall(Sema &S, const
> FunctionDecl *FD,
> if (ID == ExitID && State == FoundPathWithNoRecursiveCall)
> return;
>
> - if (State == FoundPathWithNoRecursiveCall) {
> - // If the current state is FoundPathWithNoRecursiveCall, the successors
> - // will be either FoundPathWithNoRecursiveCall or FoundPath. To
> determine
> - // which, process all the Stmt's in this block to find any recursive
> calls.
> - for (const auto &B : Block) {
> - if (B.getKind() != CFGElement::Statement)
> - continue;
> -
> - const CallExpr *CE = dyn_cast<CallExpr>(B.getAs<CFGStmt>()->getStmt());
> - if (CE && CE->getCalleeDecl() &&
> - CE->getCalleeDecl()->getCanonicalDecl() == FD) {
> -
> - // Skip function calls which are qualified with a templated class.
> - if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(
> - CE->getCallee()->IgnoreParenImpCasts())) {
> - if (NestedNameSpecifier *NNS = DRE->getQualifier()) {
> - if (NNS->getKind() == NestedNameSpecifier::TypeSpec &&
> - isa<TemplateSpecializationType>(NNS->getAsType())) {
> - continue;
> - }
> - }
> - }
> -
> - if (const CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE)) {
> - if (isa<CXXThisExpr>(MCE->getImplicitObjectArgument()) ||
> - !MCE->getMethodDecl()->isVirtual()) {
> - State = FoundPath;
> - break;
> - }
> - } else {
> - State = FoundPath;
> - break;
> - }
> - }
> - }
> - }
> + State = nextRecursiveStateInPath(FD, Block, State);
For clarity: what I'm suggesting above is to change this call to:
if (State == FoundPathWithNoRecursiveCall)
State = getNextRecursiveState(FD, Block);
or, even better:
if (State == FoundPathWithNoRecursiveCall)
if (hasRecursiveCallInPath(FD, Block))
State = FoundPath;
In either case, this could be merged with the early exit:
if (State == FoundPathWithNoRecursiveCall)
if (ExitID == ID)
return;
if (hasRecusiveCallInPath(FD, Block)
State = FoundPath;
}
These might be better left to a follow-up though.
>
> for (CFGBlock::succ_iterator I = Block.succ_begin(), E = Block.succ_end();
> I != E; ++I)
>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
