Ok! The NFC preparatory patch is in part1, and the behavior-changing work is in part2.
No problems with check-clang.
checkForFunctionCall-part1.patch
Description: Binary data
checkForFunctionCall-part2.patch
Description: Binary data
> On Jul 22, 2015, at 4:22 PM, Duncan P. N. Exon Smith <dexonsm...@apple.com> > wrote: > > >> On 2015-Jul-22, at 14:31, Vedant Kumar <v...@apple.com> wrote: >> >>> This is a fairly large block of code to further indent. Can you >>> move this to a helper function as a separate, preparatory patch >>> with no functionality change (NFC) that returns the new value of >>> `State`? >> >> I’ve attached a patch which separates the state update logic in >> checkForFunctionCall into a helper routine. >> >> I’ll send a follow-up patch with the rest of the changes you requested >> shortly. >> >> >> <checkForFunctionCall-1.patch> >> > > >> diff --git a/lib/Sema/AnalysisBasedWarnings.cpp >> b/lib/Sema/AnalysisBasedWarnings.cpp >> index f2ff48a..60bf5a0 100644 >> --- a/lib/Sema/AnalysisBasedWarnings.cpp >> +++ b/lib/Sema/AnalysisBasedWarnings.cpp >> @@ -171,21 +171,14 @@ enum RecursiveState { >> FoundPathWithNoRecursiveCall >> }; >> >> -static void checkForFunctionCall(Sema &S, const FunctionDecl *FD, >> - CFGBlock &Block, unsigned ExitID, >> - llvm::SmallVectorImpl<RecursiveState> >> &States, >> - RecursiveState State) { >> +static RecursiveState updateRecursiveState(const FunctionDecl *FD, > > Sounds like it mutates an inout parameter, and the name doesn't describe > the context. Maybe `computeRecursiveStateForSuccessors()`? That's > rather long, so feel free to come up with something better. > >> + CFGBlock &Block, unsigned ExitID, >> + RecursiveState State) { >> unsigned ID = Block.getBlockID(); >> >> - // A block's state can only move to a higher state. >> - if (States[ID] >= State) >> - return; >> - >> - States[ID] = State; >> - >> // Found a path to the exit node without a recursive call. >> if (ID == ExitID && State == FoundPathWithNoRecursiveCall) >> - return; >> + return State; > > I think you need to leave this condition in `checkForFunctionCall()`, > since otherwise you're losing the early return. > > With that out of the function, you don't need `ExitID` here at all. > >> >> if (State == FoundPathWithNoRecursiveCall) { > > Can you convert this to an early return? > > if (State != FoundPathWithNoRecursiveCall) > return State; > > Alternatively, since this is right at the top of the function, maybe you > shouldn't call this helper at all if it's not in the right state? Then > you don't need to pass `State` in at all, just assume that it's > `FoundPathWithNoRecursiveCall`. > >> // If the current state is FoundPathWithNoRecursiveCall, the successors >> @@ -200,12 +193,12 @@ static void checkForFunctionCall(Sema &S, const >> FunctionDecl *FD, >> CE->getCalleeDecl()->getCanonicalDecl() == FD) { > > Can you reduce nesting here? I'm thinking something like: > > - const CallExpr *CE = dyn_cast<CallExpr>(B.getAs<CFGStmt>()->getStmt()); > - if (CE && CE->getCalleeDecl() && > - CE->getCalleeDecl()->getCanonicalDecl() == FD) { > + const CallExpr *CE = dyn_cast<CallExpr>(B.getAs<CFGStmt>()->getStmt()); > + if (!CE || !CE->getCalleeDecl() || > + CE->getCalleeDecl()->getCanonicalDecl() != FD) > + continue; > > would clean things up a fair bit. > >> >> // Skip function calls which are qualified with a templated class. >> - if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>( >> - CE->getCallee()->IgnoreParenImpCasts())) { >> + 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; >> + continue; >> } >> } >> } > > Please clean up the rest of the new helper function as well. There are > some `break`s that should just be `return`s, etc. > >> @@ -224,6 +217,23 @@ static void checkForFunctionCall(Sema &S, const >> FunctionDecl *FD, >> } >> } >> >> + return State; >> +} >> + >> +static void checkForFunctionCall(Sema &S, const FunctionDecl *FD, >> + CFGBlock &Block, unsigned ExitID, >> + llvm::SmallVectorImpl<RecursiveState> >> &States, >> + RecursiveState State) { >> + unsigned ID = Block.getBlockID(); >> + >> + // A block's state can only move to a higher state. >> + if (States[ID] >= State) >> + return; >> + >> + States[ID] = State; > > (This is where the return for `ID == ExitID` is missing.) > >> + >> + State = updateRecursiveState(FD, Block, ExitID, State); >> + >> for (CFGBlock::succ_iterator I = Block.succ_begin(), E = Block.succ_end(); >> I != E; ++I) >> if (*I) >> >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits