[cfe-commits] r58913 - in /cfe/trunk: Driver/PrintParserCallbacks.cpp include/clang/Basic/DiagnosticKinds.def include/clang/Parse/Action.h include/clang/Parse/DeclSpec.h include/clang/Parse/Parser.h lib/Parse/MinimalAction.cpp lib/Parse/ParseDecl

[Daniel Dunbar]

Hi Argiris,

Somewhat posthumous review. :)

On Sat, Nov 8, 2008 at 8:45 AM, Argiris Kirtzidis <akyrtzi at gmail.com> wrote:
> Implement support for C++ nested-name-specifiers ('foo::bar::x') in the Parser side.
> No Sema functionality change, just the signatures of the Action/Sema methods.

>From a performance monitoring perspective, this commit would have been
easier to inspect if broken down further. No idea how easy that would
have been, just a comment.

> +  /// TryAnnotateTypeOrScopeToken - If the current token position is on a
> +  /// typename (possibly qualified in C++) or a C++ scope specifier not followed
> +  /// by a typename, TryAnnotateTypeOrScopeToken will replace one or more tokens
> +  /// with a single annotation token representing the typename or C++ scope
> +  /// respectively.
> +  /// This simplifies handling of C++ scope specifiers and allows efficient
> +  /// backtracking without the need to re-parse and resolve nested-names and
> +  /// typenames.
> +  void TryAnnotateTypeOrScopeToken();

Can you document why we use this in some places for C and not others?
Do you expect any performance impact from using it for C?

> +  CXXScopeSpec SS;
> +  if (isTokenCXXScopeSpecifier()) {
> +    ParseCXXScopeSpecifier(SS);

I believe this can be simplified. It seems that all uses of
isTokenCXXScopeSpecifier are immediately succeeded by
ParseCXXScopeSpecifier, which itself immediately asserts on
isTokenCXXScopeSpecifier(). Why not roll these into the same routine
(preferably protected outside/inline by getLang().CPlusPlus) as
MaybeParseCXXScopeSpecifier?

>From the perspective of someone not closely following the CXX support,
I would expect that a routine like isTokenCXXScopeSpecifier should be
guarded on the outside by getLang().CPlusPlus instead of containing
the check itself. This seems to hold true for the many of the other
routines which have CXX in their name. Could this become a (style)
invariant?

>  void Parser::ParseDirectDeclarator(Declarator &D) {
> +  CXXScopeSpec &SS = D.getCXXScopeSpec();
> +  DeclaratorScopeObj DeclScopeObj(*this, SS);
> +
> +  if (D.mayHaveIdentifier() && isTokenCXXScopeSpecifier()) {
> +    ParseCXXScopeSpecifier(SS);

This is a subtle variation on the pattern used in other places, I
think it is worth drawing attention to the fact that SS is a reference
here (and why).

>   // Parse the first direct-declarator seen.
>   if (Tok.is(tok::identifier) && D.mayHaveIdentifier()) {
>     assert(Tok.getIdentifierInfo() && "Not an identifier?");
> @@ -1407,18 +1479,20 @@
>         D.SetConversionFunction(ConvType, II, OperatorLoc);
>       }
>     }
> -  } else if (Tok.is(tok::l_paren)) {
> +  } else if (Tok.is(tok::l_paren) && SS.isEmpty()) {
>     // direct-declarator: '(' declarator ')'
>     // direct-declarator: '(' attributes declarator ')'
>     // Example: 'char (*X)'   or 'int (*XX)(void)'
>     ParseParenDeclarator(D);
> -  } else if (D.mayOmitIdentifier()) {
> +  } else if (D.mayOmitIdentifier() && SS.isEmpty()) {
>     // This could be something simple like "int" (in which case the declarator
>     // portion is empty), if an abstract-declarator is allowed.
>     D.SetIdentifier(0, Tok.getLocation());
>   } else {
> -    // Expected identifier or '('.
> -    Diag(Tok, diag::err_expected_ident_lparen);
> +    if (getLang().CPlusPlus)
> +      Diag(Tok, diag::err_expected_unqualified_id);
> +    else
> +      Diag(Tok, diag::err_expected_ident_lparen); // Expected identifier or '('.
>     D.SetIdentifier(0, Tok.getLocation());
>   }

This cascaded else has a lot of conditionals which are the same
(D.mayHaveIdentifier(), SS.isEmpty(), getLang().CPlusPlus) or are
otherwise logically entangled (mutually exclusive, or implied, etc).
How much would this code be simplified if it were specialized on
getLang().CPlusPlus?

>  Parser::ExprResult Parser::ParseCastExpression(bool isUnaryExpression) {
> +  if (getLang().CPlusPlus) {
> +    // Annotate typenames and C++ scope specifiers.
> +    // Used only in C++; in C let the typedef name be handled as an identifier.

Why?

 - Daniel