[cfe-dev] ParseTag only called from ParseEnumSpecifier

[Ted Kremenek]

Hi Doug,

Thanks for the very lucid explanation.  It seems to me that because of  
the commonality with the parsing logic for C that ParseClassSpecifier  
should be moved to ParseDecl.cpp.  Completely C++-specific parsing  
logic that it calls (e.g., ParseCXXMemberSpecification) should be left  
in ParseDeclCXX.cpp.

My other thought is that "ActOnTag" should be renamed (as Steve  
suggested), and only be called for struct/unions/classes.  I propose  
then that we also have a separate action for enums, since they really  
do represent a different part of the grammar once "ActOnTag" gets  
renamed.  As Steve pointed out, it seems strange that ActOnTag is  
called from two different code paths in the parser.

Ted

On Sep 3, 2008, at 11:42 PM, Doug Gregor wrote:

> On Wed, Sep 3, 2008 at 2:36 PM, Ted Kremenek <kremenek at apple.com>  
> wrote:
>> I noticed today that ParseTag is only called by ParseEnumSpecifier.
>> In the past ParseTag was used do much of the heavy lifting of parsing
>> structs, unions, and enums, and the comments on the source still say
>> this.  Now it looks like ParseTag is only called by
>> ParseEnumSpecifier, which means most of the comments are not true and
>> that the logic could probably just be rolled into ParseEnumSpecifier
>> directly.
>
> I think I made this change, so I'll chime in.
>
>> From my understanding of the code, it appears that
>> ParseClassSpecifier (in ParseDeclCXX.cpp) now does all the parsing  
>> for
>> structs, unions, and C++ classes.  This seems a little weird to me,
>> and it isn't clear from the comments in the Parse library that this  
>> is
>> the case.
>
> That comment in ParseClassSpecifier is very C++-standards-wonk; I'll
> be happy to make that clearer.
>
>> Specifically, I have two concerns:
>>
>> (1) ParseTag now appears to be somewhat redundant and unnecessary,  
>> and
>> in some ways ill-named.  It doesn't appear to serve its original
>> purpose anymore, and thus it's logic should probably just be folded
>> into ParseEnumSpecifier.
>
> I agree.
>
>> (2) It seems strange to me that parsing structs and unions, core
>> pieces of the C language, is done in ParseClassSpecifier
>> (ParseDeclCXX.cpp).  There seems to be a lot of commonality here that
>> could possibly be refactored and moved back into ParseDecl.cpp.  This
>> is my first look at this code in a while, so I'm don't claim to
>> understand all of the design decisions here, but conceptually it just
>> seems a little confusing.
>
> The C++ class-specifier (with elaborated-type-specifier) parses a
> superset of the C struct-or-union-specifier, and in C mode
> ParseClassSpecifier falls back to parsing struct-or-union-specifier.
> It didn't make sense to me to put ParseClassSpecifier into the C
> ParseDecl.cpp, because class-specifiers aren't a C concept.
>
> While it's possible to make ParseClassSpecifier delegate to ParseTag,
> I don't think it's the right approach for the future. ParseTag (which
> parses the C concept of tags) would have to deal with some nontrivial
> C++-specific parsing details for, e.g.,
>
>  class X<int>::Y
>
> Clang isn't parsing nested-name-specifiers or template-ids, so the
> ParseClassSpecifier/ParseTag code looks a lot more similar now than it
> would look if Clang had support for nested classes or class template
> specializations. ParseTag could be made to deal with these C++-isms,
> but I don't think it should be: let the non-trivial C++ parsing stay
> in the C++ part of the parser.
>
> Also, while the C99 struct-or-union-specifier and enum-specifier
> nonterminals are similar enough to permit implementation with a single
> ParseTag function, the C++ equivalents (class-specifier,
> enum-specifier and elaborated-type-specifier) have more divergent
> grammars, so it makes sense to keep ParseClassSpecifier and
> ParseEnumSpecifier separate for C++. As more of C++ (and C++0x) gets
> implemented, the common code between these two will be a much smaller
> proportion of the actual parsing functions.
>
>> As a side note, my original interest in this code was to allow the
>> parser+actions to be able to distinguish up front between enum/ 
>> struct/
>> union/class forward declarations and definitions.
>
> FWIW, this requires arbitrary lookahead in C++ (but not in C), since
> we can't tell the difference between a class-specifier and an
> elaborated-type-specifier until we've parsed through the
> nested-name-specifier and hit the actual class identifier (or
> template-id). Of course, one could apply Argiris' pre-parser idea to
> do the disambiguation between these two nonterminals in advance.
>
>  - Doug