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

[Eli Friedman]

On Fri, Jun 20, 2008 at 7:14 AM, Argiris Kirtzidis <akyrtzi at gmail.com> wrote:
> I've attached patches against the latest trunk of Parser and Sema changes
> for C++ classes support.

Some rough comments looking over the code:

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:};".)

+    }

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).

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.)

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?

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

In Sema::ActOnIdentifierExpr:
+      // FIXME: Should this use a new expr for a direct reference, instead of
+      // turning it into "this->field" ? Or else a MemberExpr with null base ?
+      ExprResult ThisExpr = ActOnCXXThis(Loc);
+      return new MemberExpr(static_cast<Expr*>(ThisExpr.Val),
+                            true, FD, Loc, FD->getType());

I'd say this is fine, except that you should use an empty
SourceLocation for the artificial "this" so it's distinguishable from
a real MemberExpr.  clang currently uses this technique for
initializer lists.

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.)

In general, this looks like a high-quality patch.  Nice work!

-Eli