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

Chris Lattner clattner at apple.com
Tue Jun 24 10:33:04 PDT 2008

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  
+  std::stack< std::stack<LexedMethod> > LexedMethodStacks;

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

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!


More information about the cfe-dev mailing list