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

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

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

>>> // 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];

Thanks Steve,

-Chris