[cfe-dev] Patch for ContextDecl

[Steve Naroff]

A minor naming nit. I'd prefer you name it DeclContext. From my  
perspective, the Decl suffix should be reserved for actual language  
constructs.

snaroff

On Apr 3, 2008, at 3:30 PM, Argiris Kirtzidis wrote:

> Hi,
>
> Here's the summary of this patch, after incorporating suggestions:
>
> -Added ContextDecl (no TranslationUnitDecl)
> -ScopedDecl class has a ContextDecl member
> -FieldDecl class has a ContextDecl member, so that a Field or a  
> ObjCIvar can be traced back to their RecordDecl/ObjCInterfaceDecl  
> easily
> -FunctionDecl, ObjCMethodDecl, TagDecl, ObjCInterfaceDecl inherit  
> from ContextDecl. With TagDecl as ContextDecl, enum constants have a  
> EnumDecl as their context.
> -Moved Decl class to a "DeclBase.h" along with ContextDecl class
> -CurContext is handled by Sema
>
> Some comments about this approach:
>
> A ContextDecl chain ends at null and a null ContextDecl gets the  
> meaning of the global scope. I'm a bit uncomfortable with applying  
> meaning to a null value
> (instead of regarding it without meaning).I'd like to try an idea to  
> use ContextDecl for name lookup (after namespaces are added) and the  
> usage of null
> as global scope will make the code a bit less readable.
>
> For another example, this:
>
> bool isDefinedOutsideFunctionOrMethod() const {
>   if (getContext())
>     return !getContext()->isFunctionOrMethod();
>   else
>     return true;
> }
>
> seems less elegant than this:
>
> bool ScopedDecl::isDefinedOutsideFunctionOrMethod() const {
>   return isa<TranslationUnitDecl>(getContextDecl());
> }
>
>
> Any thoughts on the above ?
>
>
> Chris Lattner wrote:
>>
>> On Apr 2, 2008, at 12:57 PM, Argiris Kirtzidis wrote:
>>
>>> Chris Lattner wrote:
>>>>> NamespaceDecl itself makes little sense to be a ScopedDecl, but  
>>>>> it makes sense to give it
>>>>> a context pointer to its enclosing namespace.
>>>>
>>>> Are you sure?  I thought that namespaces do impact unqualified  
>>>> scope lookups.  Perhaps not though.  If they don't, then I agree  
>>>> it should go on Decl.
>>> I see what you mean, a ScopedDecl is something that can "shadow"  
>>> another decl, right ? It makes perfect sense then.
>>
>> Right, exactly.
>>
>>>
>>>>> Most of the non-ScopedDecls are ObjC specific but for Objective C 
>>>>> ++ they will be able to use NamespaceDecl
>>>>> context pointers to, for example, get their fully qualified name.
>>>>
>>>> The various objc decls don't interact with C++ namespaces in ObjC+ 
>>>> +.  They are truly global, effectively always being installed at  
>>>> global scope.
>>> Oh, I really should refrain from making assertions about ObjC in  
>>> the future ;)
>>
>> Hehe, I admit that I'm still learning ObjC as well.  Steve and  
>> Fariborz are certainly the gurus here :)
>>
>>>>> And if RecordDecl is made a ContextDecl (mostly useful for C++),  
>>>>> a context pointer for FieldDecl would point at
>>>>> the struct/class that defined it.
>>>>>
>>>>> What do you think ?
>>>>
>>>> Ok, you convinced me, adding it to Decl makes sense :)
>>
>>> I'm less convinced than before but at least for the "unifying  
>>> concept of context" as Ted said and for RecordDecls, maybe it's  
>>> worth it for now.
>>
>>>
>>> Another idea would be:
>>> -Make Field a ScopedDecl too (at least it makes some sense for C+ 
>>> +), and use a ContextDecl member only to ScopedDecl.
>>
>> I'm not sure about this.  Fields participate in name lookup, but  
>> aren't scoped in the "name hiding" sense.  In the "reference to  
>> instance variable" case, I think that the approach taken in  
>> Sema::ActOnIdentifierExpr for ObjC ivars is really the right  
>> approach.  With an unqualified name in either ObjC or C++ the  
>> lookup sequence basically should be:
>>
>> 1. Search "local" variables in the current function, according to  
>> their scope.
>> 2. Search instance variables in the current class for non-static  
>> methods.
>> 3. Search "global" variables outside the current function,  
>> according to their scope.
>> 4. Search "implicitly global" identifiers like ObjC interfaces.
>> 5. Search implicitly defined builtins (e.g. __builtin_cos)
>>
>> The current implementation for ObjC handles #1,#3,#4 in  
>> LookupDecl.  ActOnIdentifierExpr handles #2/#5, which is  
>> suboptimal, but I think it's the best thing.
>>
>> Attempting to handle #2 in the scope lookup could be done by making  
>> all the fielddecls into scope decls, but I don't think this is a  
>> good way to go.  In addition to bloating all the fielddecls with  
>> this, it would require pushing all the ivars onto their scope  
>> chains when entering a method: this can be very expensive for  
>> classes with lots of ivars, and requires traversing (potentially  
>> deep) inheritance hierarchies.  Given this, I think having  
>> ActOnIdentifierExpr handle this is a good solution.
>>
>> #5 is also best handled for the same reason as #2: this allows us  
>> to create builtin decls lazily, which is a big win on startup time  
>> and memory use.
>>
>>> -Remove TranslationUnitDecl because otherwise, FunctionDecl would  
>>> return TranslationUnitDecl as its context and ObjCMethodDecl would
>>> return NULL (by ContextDecl::getParent()); a bit inconsistent.
>>
>> That is fine with me.  TranslationUnitDecl has other uses, but I  
>> agree that this is somewhat strange. :)
>>
>>> The convention would be that a ContextDecl chain ends at NULL,  
>>> instead of a TranslationUnitDecl.
>>
>> Ok!
>>
>>>> Would it be possible to move ContextDecl to a ContextDecl.h file  
>>>> that Decl.h #includes?
>>> "ContextDecl.h" would require the Decl class to be defined because  
>>> the ContextDecl class uses the Decl::Kind enum. Should I move the  
>>> Decl class to a "BaseDecl.h" or something ?
>>
>> Sure, maybe DeclBase.h should have both Decl *and* ContextDecl?
>>
>> -Chris
>>
>>
> <contextdecl2.patch>_______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev