[cfe-dev] C++ PATCH: Basic inheritance syntax, semantics

[Chris Lattner]

On Apr 13, 2008, at 10:54 AM, Doug Gregor wrote:
> This patch adds very basic support for parsing and type-checking class
> inheritance in C++. It'll parse the base-specifier list, e.g.,
>
>  class D : public B1, virtual public B2 { };

Nice.  One non-technical thing I should mention: Argiris is scheduled  
to implement class-related (methods, instance vars, etc) parsing and  
sema for google summer of code, which should start in a couple(?)  
weeks.  I (as his mentor) have no problem with him working on other  
stuff if he prefers, but you guys should probably sync up so that you  
don't step on each others toes.

> and do some of the simpler semantic checks (B1 and B2 are classes;
> they aren't unions or incomplete types, etc). This patch *does not*
> introduce any representation of base classes into the AST. That (along
> with the semantic checks it enables) will be a follow-on patch.

Sounds good.

> The only interesting part of this patch is that I've completely
> removed Parser::ParseStructUnionSpecifier in favor of the new
> Parser::ParseClassSpecifier. The latter parses C++ class specifiers,
> which are a superset of the C99 struct-or-union-specifier. So there's
> no reason to keep around the C-only ParseStructOrUnionSpecifier.

Makes sense.  Please add a comment that indicates that it is used in C  
though:

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

+  // 
===-------------------------------------------------------------------- 
===//
+  // C++ 9: classes [class]
+  void ParseClassSpecifier(DeclSpec &DS);

Something like:

+  // C++ 9: classes [class] and C struct/union bodies.

or something.  Also, perhaps ParseStructUnionSpecifier/ 
ParseClassSpecifier should be renamed ParseStructUnionClassSpecifier,  
though that's pretty long :).  ParseSUCSpecifier?



+  AccessSpecifier IsAccessSpecifier();

should be 'const'.  Also, for better or worse, other 'is' predicates  
in Parser (e.g. isDeclarationSpecifier) use a lowercase 'i', please be  
consistent with that.  Also, it's somewhat strange for an 'is' method  
to return something other than bool.  Maybe this should be:  
"getAccessSpecifierIfPresent()" or something like that?



+void Parser::ParseBaseClause(DeclTy *ClassDecl)
+{
+  assert(Tok.is(tok::colon) && "Not a base clause");
+  SourceLocation ColonLoc = ConsumeToken();
+
+  Actions.ActOnBaseClause(ClassDecl, ColonLoc);
+  while (true) {
+    // Parse a base-specifier.
+    if (ParseBaseSpecifier(ClassDecl)) {
...

I know the code is unfinished, but what do you anticipate using  
'ActOnBaseClause' for?  Right now it provides a Loc for the colon to  
sema and does checking for unions.  If we don't currently care about  
the colon loc, would it make sense to do the union check in  
ActOnBaseSpecifier?  It's obviously not a big deal, but one fewer  
action is nice.

+/// ParseBaseSpecifier - Parse a C++ base-specifier.
+///

Please explain a bit more about what a base-specifier is to give quick  
intuition.  This is important for people who aren't fully immersed in  
the grammar but want to understand what the code is doing quickly.   
Something like:

+/// ParseBaseSpecifier - Parse a C++ base-specifier.  base-specifier is
+/// one entry in the base class list of a class specifier, for example:
+///    class foo : public bar, private baz {
+/// 'public bar' and 'private baz' are each base-specifiers.

We don't do this consistently in the parser, but it is very useful  
where we do (for me specifically, I highly value similar comments in  
the ObjC parts ;-)



+  // FIXME: Alternatively, parse a simple-template-id.
+  if (!Tok.is(tok::identifier)) {

Please use:

+  // FIXME: Alternatively, parse a simple-template-id.
+  if (Tok.isNot(tok::identifier)) {

it reads slightly better.

+  // We have an identifier; check whether it is actually a type.
+  if (DeclTy *BaseType = Actions.isTypeName(*Tok.getIdentifierInfo(),
+                                            CurScope)) {
...

To reduce nesting, how about:

+  // We have an identifier; check whether it is actually a type.
+  DeclTy *BaseType = Actions.isTypeName(*Tok.getIdentifierInfo(),  
CurScope);

if (BaseType == 0) {
   error
   return;
}
... handle good case...


+    // Find the complete source range for the base-specifier.
+    // FIXME: Is there a better way to
+    SourceRange Range(StartLoc);
+    unsigned RawSpecifierEnd
+      = Tok.getLocation().getRawEncoding() + Tok.getLength();
+    Range.setEnd(SourceLocation::getFromRawEncoding(RawSpecifierEnd));

You shouldn't need to do this.  The end location of a source range  
actually points to the first character of the last token.  You should  
be good with something like:

   SourceRange Range(StartLoc, Tok.getLocation());

Overall, looks great, please apply with these changes (and after the  
other part gets in of course),

-Chris