> 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
