[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
akyrtzi at gmail.com
Wed Nov 26 14:42:33 PST 2008
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
More information about the cfe-commits
mailing list