[cfe-dev] [PATCH]: Parsing for C++ classes

[Chris Lattner]

On Jun 24, 2008, at 6:28 AM, Argiris Kirtzidis wrote:

> Hi,
>
> I incorporated the excellent feedback by Chris and Eli and I'm  
> posting a new Parser patch for C++ classes.
> The main differences with the previous one are:
>
> -The C++ class parser is more simplified and "naive", and almost all  
> checks are deferred to Sema.
> -There's a new Parser::ConsumeAndStoreUntil method, instead of the  
> templated ConsumeUntil. SkipUntil is untouched.
> -New Action methods: ActOnCXXMemberDeclarator and  
> ActOnFinishCXXMemberSpecification.

This is looking great! Some questions:

+  /// LexedMethodStacks - During parsing of a top (non-nested) C++  
class, its
+  /// inline method definitions and the inline method definitions of  
its nested
+  /// classes are lexed and stored here.
+  /// A new lexed methods stack is pushed for local classes in inline  
methods.
+  std::stack< std::stack<LexedMethod> > LexedMethodStacks;

Out of curiousity, why use a std::stack instead of std::deque or  
std::vector?

For clarity, it might be better to make a typedef for the inner stack,  
something like:

   typedef std::stack<LexedMethod> MethodTokensForOneClass;

or something?  Use of that typedef would make the interfaces a bit  
more clear.

+  void PushLexedMethodStack() {
+  void PopLexedMethodStack() { LexedMethodStacks.pop(); }

These really push one 'nested class' worth of methods, right?  It  
might be better to name these "PushClassStack" or something.



+++ lib/Parse/ParseCXXInlineMethods.cpp	(revision 0)

A new file is a great idea.


+ Parser::ParseCXXInlineMethodDef(AccessSpecifier AS, Declarator &D) {
+  assert(D.getTypeObject(0).Kind == DeclaratorChunk::Function &&
+         "This isn't a function declarator!");
+
+  DeclTy *FnD = Actions.ActOnCXXMemberDeclarator(CurScope, AS, D, 0,  
0, 0);
+
+  // We should have an opening brace now.
+  if (Tok.isNot(tok::l_brace)) {

The only caller of this knows that the current token is l_brace, so  
this can be an assert.



In Parser::ConsumeAndStoreUntil, do you think it would make sense to  
specialize it for T=r_brace, or do you expect other clients?

Other than these minor issues, the patch looks great to me.  If Eli is  
happy with it, please apply!  Great job Argiris!

-Chris