[cfe-dev] Patch for ContextDecl

[Chris Lattner]

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

> Hi,
>
> Here's the summary of this patch, after incorporating suggestions:

This is excellent progress.  I've checked it in, even though there are  
still todo's :)

> -Added ContextDecl (no TranslationUnitDecl)

I think steve has a good point, it would be nice to rename this to  
DeclContext.

> -ScopedDecl class has a ContextDecl member

Ok.

> -FieldDecl class has a ContextDecl member, so that a Field or a  
> ObjCIvar can be traced back to their RecordDecl/ObjCInterfaceDecl  
> easily

I'm not so sure about this one :).  Ignoring ObjC for the moment,  
Steve and I are quite concerned about the size of two classes in  
particular: enumconstants and fielddecls.  These occur by the  
thousands in common large headers, so they quickly dominate memory use  
for crazy header cases. I'll ignore enums for now, which are scope  
decls, so there isn't much we can do.

C++ has a lot of interesting needs for structs: you need context info  
(for methods), a parent class pointer, information about accessibility  
(public/private) etc.  Adding all of this state to RecordDecl seems  
like a bad way to go if we can avoid it: it bloats the AST for plain C  
code, and all those fields are dead.  In addition, many structs/unions  
and even C++ classes don't need *any* of this extra information if  
they are sufficiently 'pod like'.  For C++ apps that #include lots of  
C headers, for example, significant space could be saved by not having  
this state for those structs.

What do you think of introduce a new RecordDecl subclass:  
CXXRecordDecl, which provides space for all this extra stuff.  Once  
the parser has seen the '}' in the record definition, it can know  
whether the 'struct' is sufficiently simple to be cast as a RecordDecl  
(which is always true for c/objc, and sometimes true for C++).  If  
not, it creates a CXXRecordDecl.

If we go down this path, I think it would make sense for CXXRecordDecl  
to be a DeclContext, but not for RecordDecl to be.  What do you think?

> -FunctionDecl, ObjCMethodDecl, TagDecl, ObjCInterfaceDecl inherit  
> from ContextDecl. With TagDecl as ContextDecl, enum constants have a  
> EnumDecl as their context.

I think this makes sense, what do you think Steve?

> -Moved Decl class to a "DeclBase.h" along with ContextDecl class
> -CurContext is handled by Sema

Thanks!

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

I agree, I think it would be nice to add back TranslationUnitDecl:  
it's a nice unifying concept.  There are two ways to solve the  
"function has a context but ObjCInterface doesn't" issue: one is to  
just add the pointer to objcinterface, the second is to just say "that  
is the way it should be".

Another next step: what do you think of hoisting  
FunctionDecl::DeclChain to be a member of ContextDecl?  This would  
allow clients to iterate over all decls in a context, likewise for  
EnumDecl::ElementList etc?

-Chris