[cfe-commits] r93287 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Analysis/AnalysisContext.cpp lib/Sema/Sema.h lib/Sema/SemaDecl.cpp lib/Sema/SemaExpr.cpp test/SemaCXX/unreachable-code.cpp

[Ted Kremenek]

On Jan 12, 2010, at 6:59 PM, Mike Stump wrote:

> Author: mrs
> Date: Tue Jan 12 20:59:54 2010
> New Revision: 93287
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=93287&view=rev
> Log:
> Add an unreachable code checker.
> 
> ==============================================================================
> -
> +/// CheckUnreachable - Check for unreachable code.
> +void Sema::CheckUnreachable(AnalysisContext &AC) {
> +  if (Diags.getDiagnosticLevel(diag::warn_unreachable) == Diagnostic::Ignored)
> +    return;
> +
> +  CFG *cfg = AC.getCFG();
> +  // FIXME: They should never return 0, fix that, delete this code.
> +  if (cfg == 0)
> +    return;
> +  
> +  llvm::BitVector live(cfg->getNumBlockIDs());
> +  // Mark all live things first.
> +  MarkLive(&cfg->getEntry(), live);
> +
> +  for (unsigned i = 0; i < cfg->getNumBlockIDs(); ++i) {
> +    if (!live[i]) {
> +      CFGBlock &b = *(cfg->begin()[i]);
> +      if (!b.empty())
> +        Diag(b[0].getStmt()->getLocStart(), diag::warn_unreachable);
> +      // Avoid excessive errors by marking everything reachable from here
> +      MarkLive(&b, live);
> +    }
> +  }
> +}

Hi Mike,

We talked about this in person a bit, but I thought I'd shoot you a few comments.

I think this loop should just iterate over the CFGBlocks using the CFG's iterator instead of iterating over block ID numbers.  The following lines look a little fragile:

> +  for (unsigned i = 0; i < cfg->getNumBlockIDs(); ++i) {
> +    if (!live[i]) {
> +      CFGBlock &b = *(cfg->begin()[i]);

This makes the unnecessary assumption that the iterator is random access.  While true, this doesn't look necessary.  How about:

for (CFG::iterator I = cfg->begin(), E = cfg->end(); I != E; ++I) {
   CFGBlock *b = *I;
   if (!live[b->getBlockID()])

This is a little easier to read and makes it algorithmically clearer what you're doing.  As we discussed, this probably isn't the final solution anyway, since it won't suppress errors as expected (i.e., the CFGBlocks are not guaranteed to be in topological order), but from a coding perspective this is much cleaner.

- Ted
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100113/1a2437e8/attachment.html>