[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 9:38 PM, Chris Lattner wrote:

> On Feb 28, 2009, at 1:05 PM, steve naroff wrote:
>>> 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.
>
> It is not amazingly expensive (the first malloc happens on the first  
> insert) but a densemap is 4 words, so this significantly increases  
> the size of Scope.
>
>> I don't have a strong opinion. Just let me know what you prefer and  
>> I'll take care of it.
>
> I agree with you that #1 is gross.  #2 seems reasonable.
>

O.K., however the next issue is related and raises a 3rd alternative.

>>
>>
>>>>
>>>> +  /// 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).
>
> Ok, you removed PrevFunctionScope (yay!) but kept ActiveScope.  What  
> is ActiveScope? Can it be removed?

ActiveScope allows ActOnLabelStmt/ActOnAddrLabel/ActOnGotoStmt/ 
ActOnFinishFunctionBody to perform their operations (generically) on  
the respective function/method/block.

If we decided to remove it, the ActOn label-related functions above  
*could* use CurBlock to decipher it's context (making the actions a  
bit more complex). If we decided to do this, the LabelMap in Scope  
would need to be in 2 places: One in BlockSemaInfo (for the block  
stack), and one in Sema (for functions, like we had prior to this bug  
fix). From a performance perspective, this makes the most sense (since  
it places the overhead where it belongs).

>
>
>>>> +  // 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.
>
> Ok, but why?

Because the ActOnStartOfFunctionDef/ActOnFinishFunctionBody don't  
implement a stack (since C/ObjC functions never nest). Not difficult  
to fix.

We could consider morphing BlockSemaInfo into NestedFunctionInfo  
(where blocks would be a client).

>
>
>>>> // 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 *"?
>
> Yes, use:
>
> Action::StmtTy *&LabelDecl = ActiveScope->LabelMap[II];
>

O.k. I'll see if that simplifies the code...

snaroff

> Thanks Steve,
>
> -Chris
>