[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

[steve naroff]

On Feb 28, 2009, at 2:48 PM, Chris Lattner wrote:

>
> 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?
>

I considered #1, however it seemed cleaner to leverage the existing  
Scope mechanism (which is a catch-all by design).

Is the cost to setup an empty DenseMap much greater than setting up an  
empty SmallVector? If so, then I guess I'd prefer #2.

I don't have a strong opinion. Just let me know what you prefer and  
I'll take care of it.

>>
>> +  /// 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.
>

Just committed a fix for this (good catch).

>> +  // 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?

The code was crashing because ActiveScope was null.

>
>
>> +++ 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.

I tried doing this, however C++ casting seemed to get in my way. Is it  
possible to do what you want when the LabelMap is declared to take a  
"void *"?

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