rsmith added a comment.
OK, so it seems like all the other approaches we discussed have problems too.
Let's move forward with this for now.
Is there some reasonable base set of functionality between this and
JumpDiagnostics that could be factored out and shared?
> VarBypassDetector.cpp:45
> +
> +/// BuildScopeInformation - Walk through the statements, adding any labels or
> +/// gotos to LabelAndGotoScopes and recursively walking the AST as needed.
Drop the `BuildScopeInformation -` here and in other documentation comments.
> VarBypassDetector.cpp:50-51
> + // If this is a statement, rather than an expression, scopes within it
> don't
> + // propagate out into the enclosing scope. Otherwise we have to worry
> + // about block literals, which have the lifetime of their enclosing
> statement.
> + unsigned independentParentScope = origParentScope;
Comment seems to not be relevant in this copy of the code; we don't have any
special case for block literals here.
> VarBypassDetector.cpp:82-86
> + case Stmt::CaseStmtClass:
> + case Stmt::DefaultStmtClass:
> + case Stmt::LabelStmtClass:
> + LabelAndGotoScopes[S] = ParentScope;
> + break;
This looks unreachable (our only callers are the recursive call below -- which
already checked for these -- and cases that cannot have a labelled statement).
If we did reach it, we'd do the wrong thing, because we'd have created an
independent scope rather than reusing the parent's scope (`x: int n;` would not
introduce a new scope into the parent for `n`). Maybe replace this case with
`llvm_unreachable`, or move the `while (true)` loop below up to the top of this
function and delete these cases.
Do we have the same issue in JumpDiagnostics?
> VarBypassDetector.cpp:105-108
> + if (const CaseStmt *CS = dyn_cast<CaseStmt>(SubStmt))
> + Next = CS->getSubStmt();
> + else if (const DefaultStmt *DS = dyn_cast<DefaultStmt>(SubStmt))
> + Next = DS->getSubStmt();
Combine these two into a single `if (auto *SC = dyn_cast<SwitchCase>(SubStmt))`
case.
> VarBypassDetector.cpp:129
> + if (const LabelStmt *LS = GS->getLabel()->getStmt())
> + Detect(GS, LS);
> + } else if (const SwitchStmt *SS = dyn_cast<SwitchStmt>(St)) {
Maybe pass in `S.second` instead of `GS` here so that the callee doesn't need
to look it up again.
> VarBypassDetector.h:39
> +/// * Not limited to variables with initializers, JumpScopeChecker is
> limited.
> +/// * FIXME: Does not support indirect jumps.
> +class VarBypassDetector {
We should do /something/ about these. We could track the set of address-taken
labels when we walk the function, and assume that each indirect goto can jump
to each address-taken label, or more simply just conservatively say that all
variables are bypassed in a function that contains an indirect goto.
https://reviews.llvm.org/D24693
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
