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

[Argiris Kirtzidis]

Hi Chris,

Chris Lattner wrote:
>
> +++ 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.

I don't quite see how the specialized version will be different from 
SkipUntil. You still need to consider braces,parens,brackets, strings 
etc. Basically the implementation of SkipUntil will be copied.
Is it ok to duplicate the code for this ?

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

Ok, I was under the impression that if the parser has the necessary 
information to determine an error, it should emit the error.
Is the parser supposed to defer to Sema as many checks as 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);

Ok to all.

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

Yes, the check handles nested classes too.

>
>
> Very picky but:
>
> +      assert(I->Toks.size() > 0 && "Empty body!");
>
> How about !I->Toks.empty()

Ok.

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

Yes, after the method is parsed, the current token is the previously 
pushed token, and it needs to be pushed again.

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

Static methods and C functions are checked on Sema since the parser 
don't have this kind of information.
This applies to my previous question about where should the checks be 
handled.

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

Ok, good idea.


-Argiris