[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

[Argiris Kirtzidis]

Hi Daniel,

Daniel Dunbar wrote:
> >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.
>   

Sorry for the huge commit.
It's really awesome that you track performance regressions! Are there 
still issues after your commit in r60063 ?

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

Added comments in r60119.

> Do you expect any performance impact from using it for C?
>   

No, I don't think so.

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

Done in r60117.

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

Done (MaybeParseCXXScopeSpecifier is guarded).

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

Changed this piece of code in r60124.

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

Added comments in r60119.

-Argiris