[cfe-dev] [PATCH]: Preparing AST for C++ declarations

[Eli Friedman]

On Wed, Jun 4, 2008 at 7:22 AM, Argiris Kirtzidis <akyrtzi at gmail.com> wrote:
> Hi Eli,
> thanks for reviewing.
>
> Eli Friedman wrote:
>>
>> >From my reading, there's nothing obviously wrong with this patch,
>> except that it's too big, which makes reviewing it a lot more
>> difficult.  Please put the DeclBase code movement into a separate
>> patch (moving the DeclBase code is fine without review), put the
>> DeclChain changes into a separate patch, put the Decl::Struct ->
>> TagDecl::TK_struct changes into a separate patch, and split out
>> anything else that's obviously independent.
>>
>
> I've added DeclBase.cpp here (Chris was fine with this change at the
> previous related patch):
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080602/006009.html
>
> In the currently attached patch there are only the AST-related changes
> (introducing the C++ decl subclasses); these are the important ones.

Okay, thanks; this is a lot easier to look at.

>> Minor issue I spotted: in CXXMethodDecl::getThisType, there's no point
>> to making "this" const; it isn't an lvalue, so the const modifier
>> doesn't do anything.
>>
>
> Making 'this' const is so that the error diagnostics can report its type as
> "[class name]* const". This convention is followed by both gcc and msvc.

All right, I guess.


Most important issue: you're adding an Access member to TypeDecl, a
class which isn't C++-specific.  This will increase the size of all
TypeDecls for C code by 4 bytes.  Adding new members to data
structures requires additional scrutiny, because it affects memory
usage.  In this case, it doesn't appear to be worthwhile to bloat the
memory usage for C code.  There are a couple of possible ways to fix
this: the simplest is to add a CXXEnumDecl and CXXTypedefDecl, and
shuffle the Access member down to the C++-specific classes.  This
isn't exactly pretty, but it's straightforward to implement.

Have you considered what will happen to RecordDecls once inheritance
is implemented?  I'm not sure that a RecordDecl will continue to
properly make sense, at least in terms of the internal data structure.
 The Nth member of a RecordDecl might not really have any
correspondence to anything useful, although I guess most code
iterating over members needs to be aware of C++ anyway.  Member name
lookup will also need to be completely different.  I guess it isn't
really an issue at this point, but something to think about.

A CXXInstanceMethodType and CXXStaticMethodType are going to be
necessary at some point; also not something that needs to be done
here, but something to consider.

I notice you didn't include the serialization implementation; it's
okay to leave it out for now, but it will need to be done.

Overall, the patch seems to be well thought out.  I'm not completely
sure everything is going to remain the same once some of this stuff
actually has parser/sema support, but it's hard to tell without
implementing it.

-Eli