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

[Doug Gregor]

On Sun, Apr 13, 2008 at 4:19 PM, Chris Lattner <clattner at apple.com> wrote:
> 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.

I'm happy to leave the class-related bits to Argiris. Thanks for the heads-up.

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

Okay.

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

Yuck. In C++-speak, it's just a "class specifier" because classes,
structs, and unions are all class types. That's why I prefer the name
ParseClassSpecifier (but the comment is helpful).

>  +  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?

I like getAccessSpecifierIfPresent; let's go with 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.

I don't anything else planned for ActOnBaseClause... the union check
could move into ActOnBaseSpecifier.

>  +/// 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
> ;-)

Okay.

>  +  // 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...

Okay.

>  +    // 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.

... and that token is still highlighted with the source range. I
didn't realize that. Thanks!

FWIW, the second patch I sent just added the regression test and fixed
a typo or two. Nothing that requires a separate review.

  - Doug