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

[Chris Lattner]

On Jun 20, 2008, at 7:14 AM, Argiris Kirtzidis wrote:

> I've attached patches against the latest trunk of Parser and Sema  
> changes for C++ classes support.

Wow, this is looking very nice Argiris.  Most of the stuff below is  
nit picky details, you're making excellent progress.

> The Parser->Sema diff for "cxx-sema.patch" was kinda manually  
> created to make reviewing just the Sema changes easier, so you  
> cannot apply it. If you want to try out Parser+Sema changes you need  
> to apply the "cxx-parser-sema.patch", which contains the unified diff.

I'll start with the parser pieces.  Once those go in I'll take a look  
at sema pieces, thanks!  Incidentally, the plan is to get more caught  
up on recent clang development this weekend.  Here's hoping :)

> Here's a few more details in addition to the summary for the parser  
> changes which I mentioned in the previous post:
>
> When the parser encounters an inline method definition, it lexes and  
> stores its tokens so that it can parse it just after the parsing of  
> the topmost class. I.e:
>
> class C {
>  void m1() {}
>   class NC {
>      void m2() {}
>  };
> };
>
> The parsing of the above class is something like this:
>
> class C {
>  void m1();
>   class NC {
>      void m2();
>  };
> };
>
> void m1() {}  // of class C
> void m2() {}  // of class C::NC

Right.

> For local classes, it would be like a function is defined inside  
> another function. The Action module is supposed to be aware of this  
> and allow it if the function being defined is an inline class method.

Makes sense.

Details:

+DIAG(err_member_initialization, ERROR,
+     "member '%0' can be initialized only if it is a static const  
integral data member")

80 cols please :)



+++ include/clang/Parse/Parser.h	(working copy)

   template <typename ConsumeFunc>
+  bool ConsumeUntil(tok::TokenKind T, ConsumeFunc &ConFn, bool  
StopAtSemi = true,
+                 bool DontConsume = false) {
+    return ConsumeUntil(&T, 1, ConFn, StopAtSemi, DontConsume);
+  }
+
+  template <typename ConsumeFunc>
+  bool ConsumeUntil(const tok::TokenKind *Toks, unsigned NumToks,
+                 ConsumeFunc &ConFn, bool StopAtSemi, bool  
DontConsume) {


I'm fairly uncomfortable with making ConsumeUntil completely  
templated.  In addition to inviting code bloat, this requires a bunch  
of implementation details to be in the header.  Can this be changed to  
take a function pointer (possibly with a void* data pointer) instead?

It looks like you're using this for "method body eating".  It would  
also be reasonable to have a specialized version of this that just  
counts braces/parens etc.  I consider SkipUntil to really be part of  
the diagnostic machinery... having it be used by the parser is a bit  
strange to me, and could make future extensions to it more difficult.



+///         ::[opt] nested-name-specifier template[opt] unqualified- 
id ';' [TODO]

80 columns.



+  // C++ 9.2p6: A member shall not be declared to have automatic  
storage
+  // duration (auto, register) or with the extern storage-class- 
specifier.
+  switch (DS.getStorageClassSpec()) {
+    case DeclSpec::SCS_unspecified:

Should this code live in parser or in sema?  It seems cleaner to keep  
the 'analysis' code in sema as much as possible.  I know we aren't  
necessarily clean about this everywhere, but I think it would be  
useful to avoid this if possible.


+      } else if (DS.getStorageClassSpec() == DeclSpec::SCS_static) {
+        // "static member 'A' cannot be a bit-field"
+        Diag(colonLoc, diag::err_static_not_bitfield,
+             DeclaratorInfo.getIdentifier()->getName());
+        SkipUntil(tok::comma, true, true);
+
+      } else {
+        assert(0 && "Didn't we cover all member kinds?");

How about:


+      } else {
+        assert(DS.getStorageClassSpec() == DeclSpec::SCS_static &&
+               "Didn't we cover all member kinds?");
+        // "static member 'A' cannot be a bit-field"
+        Diag(colonLoc, diag::err_static_not_bitfield,
+             DeclaratorInfo.getIdentifier()->getName());
+        SkipUntil(tok::comma, true, true);
+
+      }

which is faster in release-asserts mode and doesn't have a false path.


+  // C++ 11p3: Members of a class defined with the keyword class are  
private
+  // by default. Members of a class defined with the keywords struct  
or union
+  // are public by default.
+  AccessSpecifier CurAS =
+    TagType == DeclSpec::TST_struct ||
+    TagType == DeclSpec::TST_union     ? AS_public
+                                       : AS_private;

The precedence here is non-obvious, please use an if/then/else instead  
of ?: or parenthesize.


+      if (Tok.is(tok::colon))
+        ConsumeToken();
+      else
+        Diag(Tok.getLocation(), diag::err_expected_colon);

How about:
   ExpectAndConsume(tok::colon, diag::err_expected_colon);


+  // C++ 9.2p2: Within the class member-specification, the class is  
regarded as
+  // complete within function bodies, default arguments,
+  // exception-specifications, and constructor ctor-initializers  
(including
+  // such things in nested classes).
+  //
+  // FIXME: Only function bodies are parsed correctly, fix the rest.
+  if (!(CurScope->getFlags() & Scope::CXXClassScope)) {

Is this check sufficient for nested classes?  If not, please add a  
fixme.


Very picky but:

+      assert(I->Toks.size() > 0 && "Empty body!");

How about !I->Toks.empty()


+      // Append the current token at the end of the new token stream  
so that it
+      // doesn't get lost.
+      I->Toks.push_back(Tok);
+      PP.EnterTokenStream(&I->Toks.front(), I->Toks.size(), true,  
false);

Very nice!  If there are multiple methods, should this push Tok for  
all of them?

+Parser::ExprResult Parser::ParseCXXThis() {
+  assert(Tok.is(tok::kw_this) && "Not 'this'!");
+  SourceLocation ThisLoc = ConsumeToken();
+  if (CurScope->getFnParent() == 0) {
+    Diag(ThisLoc, diag::err_invalid_this_at_top_level);
+    return ExprResult(true);
+  }

Should this also check for static methods?  What about C functions?   
Alternatively, does sema handle these checks?  If so, should Sema  
handle all the checks?


+/// TryParseFunctionOrCXXMethodDef - Check the parsed declarator and  
continue
+/// parsing if it is a function definition.
+/// If it is a function definition it will return true and 'Res' will  
hold the
+/// declaration.
+/// If it is not a function definition it will return false.
+/// If a parsing error occured, it will return true and 'Res' will  
hold 0.

Please mention that on a parse error, no tokens will be consumed.

+bool Parser::TryParseFunctionOrCXXMethodDef(Declarator &DeclaratorInfo,
+                                            DeclTy *&Res) {


This returns a Decl plus a "success" bool.  Should this return a  
"DeclResult" like the existing ExprResult/StmtResult etc stuff used by  
actions?


80 cols:

-  Decl *decl = static_cast<Decl*>(ActOnDeclarator(GlobalScope, D, 0));
+
+  return ActOnStartOfFunctionDef(FnBodyScope,  
ActOnDeclarator(GlobalScope, D, 0));

-Chris