[cfe-dev] Patch for ContextDecl

[Argiris Kirtzidis]

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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: contextdecl2.patch
Type: text/x-diff
Size: 77717 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080403/0ad00386/attachment.patch>