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

[Argiris Kirtzidis]

Hi Eli, thanks for reviewing.

Eli Friedman wrote:
> In Parser::ParseCXXMemberSpecification:
> +    AccessSpecifier AS = getAccessSpecifierIfPresent();
> +    if (AS != AS_none) {
> +      // We encountered a C++ access specifier.
> +      CurAS = AS;
> +      ConsumeToken();
> +
> +      if (Tok.is(tok::colon))
> +        ConsumeToken();
> +      else
> +        Diag(Tok.getLocation(), diag::err_expected_colon);
>
> I think you forgot to put a "continue;" here.  (Consider the testcase
> "class A {public:private:int a;public:};".)
>
> +    }
>   

Oops, thanks for noticing!

> In Parser::ParseCXXClassMemberDeclaration:
> You mention the pure specifier in the comments, but it looks like you
> forgot to actually implement it (or at least put in a FIXME where it
> should be implemented).
>   

I've put a [TODO] for the pure specifier on the top of 
ParseCXXClassMemberDeclaration. And I've put

>         // Defer all other checks to Sema::AddInitializerToDecl.
which is where it is supposed to be handled. Shall I add a more specific 
comment for pure specifier ?
Also note that initializers are not handled at the moment.

> In Parser::SkipUntil:
> Standard large patch advice: split out changes that can be easily
> split out.  (Here, the change isn't especially useful without the rest
> of the patch, but it makes review easier and the svn history a bit
> cleaner.)
>   

I'll commit the change to SkipUntil in a separate patch if the consensus 
is that this is the way to go for lexing the inline method definitions.

> In Parser::TryParseFunctionOrCXXMethodDef:
> +      Tok.is(tok::colon) ||           // int X():  -> not a function def
> Hmm?  Doesn't a colon guarantee that this is a function definition
> with a ctor-initializer?  Or are you planning on handling this case
> some other way?
>   

I think that constructors will be handled in some other way; as I see 
it, this method will be used on declarators with declaration specifiers.
If it turns out that it'd be useful for constructors to go through this 
method, we can always change it later.

> -              isDeclarationSpecifier())) {      // int X(f) int f; {}
> +              (!getLang().CPlusPlus &&
> +               isDeclarationSpecifier()))) {    // int X(f) int f; {}
> This change should be split out.
>   

Ok, this will be in a different commit.

> About the Sema Decl context stuff:
> It is in fact possible to nest Function/MethodDecls with C++.  Simple example:
> int a() {
>   static int b;
>   class C {
>     public:
>     int a() {
>       return b;
>     }
>   } r;
>   return r.a();
> }
>
> If I'm not mistaken, member() should be parsed in the context of
> func().  (This is somewhat evil/not especially useful, but as far as I
> can tell, it's legal.)
>   

You are referring to
>     assert(CurFunctionDecl == 0 && "Confused parsing.");
right ?
I agree, thanks again for noticing!


-Argiris