[cfe-commits] r65694 - in /cfe/trunk: include/clang/Parse/Scope.h lib/Sema/Sema.cpp lib/Sema/Sema.h lib/Sema/SemaDecl.cpp lib/Sema/SemaDeclObjC.cpp lib/Sema/SemaExpr.cpp lib/Sema/SemaStmt.cpp test/Sema/block-labels.c

[Chris Lattner]

On Feb 28, 2009, at 8:48 AM, Steve Naroff wrote:

> Author: snaroff
> Date: Sat Feb 28 10:48:43 2009
> New Revision: 65694
>
> URL: http://llvm.org/viewvc/llvm-project?rev=65694&view=rev
> Log:
> Fix <rdar://problem/6451399> problems with labels and blocks.
>
> - Move the 'LabelMap' from Sema to Scope. To avoid layering  
> problems, the second element is now a 'StmtTy *', which makes the  
> LabelMap a bit more verbose to deal with.
> - Add 'ActiveScope' to Sema. Managed by ActOnStartOfFunctionDef(),  
> ObjCActOnStartOfMethodDef(), ActOnBlockStmtExpr().
> - Changed ActOnLabelStmt(), ActOnGotoStmt(), ActOnAddrLabel(), and  
> ActOnFinishFunctionBody() to use the new ActiveScope.
> - Added FIXME to workaround in ActOnFinishFunctionBody() (for  
> dealing with C++ nested functions).

Thanks for working on this Steve.  I somewhat concerned about the cost  
of this though: scopes are very frantically pushed and popped, and  
adding a densemap to them seems potentially expensive.  Two possible  
solutions: 1) use a separate explicit stack for these, or 2) make  
scope contain a pointer to a (lazily initialized) densemap instead of  
containing it by value.  #2 would only penalize scopes that actually  
contain labels which are pretty rare.

What do you think?

>
> +  /// ActiveScope - If inside of a function, method, or block  
> definition,
> +  /// this contains a pointer to the active scope that represents it.
> +  Scope *ActiveScope;

> +  /// PrevFunctionScope - This is the scope for the enclosing  
> function.
> +  /// For global blocks, this will be null.
> +  Scope *PrevFunctionScope;

How are these different?  Have you seen that Scope has a bunch of  
things like FnParent etc that make getting this information very  
cheap.  It seems best to use these pointers because they are  
guaranteed up to date and don't require new code to maintain them.

> +  // FIXME: Temporary hack to workaround nested C++ functions. For  
> example:
> +  // class C2 {
> +  //   void f() {
> +  //     class LC1 {
> +  //       int m() { return 1; }
> +  //     };
> +  //   }
> +  // };
> +  if (ActiveScope == 0)
> +    return D;
> +

I don't understand the issue here.  Why doesn't this work?

> +++ cfe/trunk/lib/Sema/SemaStmt.cpp Sat Feb 28 10:48:43 2009
> @@ -165,12 +165,19 @@
> Sema::ActOnLabelStmt(SourceLocation IdentLoc, IdentifierInfo *II,
>                      SourceLocation ColonLoc, StmtArg subStmt) {
>   Stmt *SubStmt = static_cast<Stmt*>(subStmt.release());
> +
>   // Look up the record for this label identifier.
> -  LabelStmt *&LabelDecl = LabelMap[II];
> +  Scope::LabelMapTy::iterator I = ActiveScope->LabelMap.find(II);
>
> +  LabelStmt *LabelDecl;
> +
>   // If not forward referenced or defined already, just create a new  
> LabelStmt.
> +  if (I == ActiveScope->LabelMap.end()) {
> +    LabelDecl = new (Context) LabelStmt(IdentLoc, II, SubStmt);
> +    ActiveScope->LabelMap.insert(std::make_pair(II, LabelDecl));
> +    return Owned(LabelDecl);
> +  } else
> +    LabelDecl = static_cast<LabelStmt *>(I->second);

Instead of using find+insert, please use the old idiom of constructing  
a reference into the map, something like:

LabelStmt *&LabelDecl = ActiveScope->LabelMap[II];
if (LabelDecl == 0) {
   LabelDecl = new (Context) LabelStmt(IdentLoc, II, SubStmt);
   ActiveScope->LabelMap.insert(std::make_pair(II, LabelDecl));
   return Owned(LabelDecl);
}

This eliminates a lookup for the insert.

>   // Look up the record for this label identifier.
> -  LabelStmt *&LabelDecl = LabelMap[LabelII];
> +  Scope::LabelMapTy::iterator I = ActiveScope- 
> >LabelMap.find(LabelII);

likewise here.

-Chris