[cfe-dev] ParseTag only called from ParseEnumSpecifier

steve naroff snaroff at apple.com
Wed Sep 3 12:12:20 PDT 2008


I agree this should be cleaned up. Comments below...

On Sep 3, 2008, at 2:36 PM, Ted Kremenek 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.
>
> 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.  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.
>

Another solution is to have ParseClassSpecifier() call ParseTag(). At  
the moment, both of these routines call "ActOnTag" (which is a little  
odd). If ParseClassSpecifier() called ParseTag(), the action could be  
sent from one place. It also means we could set the "TagKind" (Def,  
Decl, Ref) in one place. From my perspective, centralizing both of  
these would be simplifying.

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

Right...if ParseClassSpecifier() called ParseTag() it would make more  
sense. I haven't looked that closely, however I imagine it could be  
refactored to do this.

Based on my previous comment, I guess I'd prefer we make ParseTag()  
more reusable (rather than get rid of it).

Thoughts?

snaroff

> 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.  This would allow us
> to hopefully clean up some elements of the ASTs with regards to
> forward declarations of tag types, and it seems it would also relay
> more information to the parser Actions.
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev




More information about the cfe-dev mailing list