[cfe-dev] [PATCH] C++ nested-names (Parser) and annotation tokens

[Argiris Kirtzidis]

Hi Doug, thanks for reviewing.

Doug Gregor wrote:
> First of all, I'm really impressed. This came out much cleaner than I
> had anticipated. I have some nitpicks and a question below, but as far
> as I'm concerned, if you have tests and some minimal Sema support (so
> that all the tests run properly), please feel free to check it in.
> Great job!
>
> Some nitpicks follow.
>
> +  /// ActOnCXXEnterDeclaratorScope - Called when a C++ scope specifier (global
> +  /// scope or nested-name-specifier) is parsed, part of a declarator-id.
> +  /// After this method is called, according to [C++ 3.4.3p3], names should be
> +  /// looked up in the declarator-id's scope, until the declarator is
> parsed and
> +  /// ActOnCXXExitDeclaratorScope is called.
> +  /// The 'SS' should be a non-empty valid CXXScopeSpec.
> +  virtual void ActOnCXXEnterDeclaratorScope(Scope *S, const CXXScopeSpec &SS) {
> +  }
> +
> +  /// ActOnCXXExitDeclaratorScope - Called when a declarator that previously
> +  /// invoked ActOnCXXEnterDeclaratorScope(), is finished. 'SS' is the same
> +  /// CXXScopeSpec that was passed to ActOnCXXEnterDeclaratorScope as well.
> +  /// Used to indicate that names should revert to being looked up in the
> +  /// defining scope.
> +  virtual void ActOnCXXExitDeclaratorScope(const CXXScopeSpec &SS) {
> +  }
>
> It would be really, really great if we had an RAII class of some sort
> to make sure that
> ActOnCXXEnterDeclaratorScope/ActOnCXXExitDeclaratorScope calls were
> always paired. For example, Parser::ParseDirectDeclarator's control
> flow looks right, but the Enter call is pretty far from the Exit call
> (and at a different level of "if" nesting).
>   

Good idea!

> I'm pretty certain that I don't understand the role of these
> functions. It looks like they want Sema to keep some additional state
> regarding the scope in which name lookup should occur that's somehow
> different from the current scope of parsing, but I don't know when
> that happens... when we're parsing a direct-declarator, say, when
> declaring a member function out-of-line:
>
>   void Sema::MemberFunction(TypeFromInSema Arg) { }
>
> Once we've parse Sema::MemberFunction, we seem to be calling
> ActOnCXXEnterDeclaratorScope. I assume that's so that the lookup of
> TypeFromInSema will look into 'Sema' first?
>   

Yes, and here's another example:

class C {
  static const int num=5;
  static int arr[num];
  void m();
};

int C::arr[num]; // lookup 'num' in 'class C' scope

> Could we instead have a different kind of Scope that handles this case
> of being inside "Sema" without
>   

This is a "enter into an arbitrary already existing scope" kind of 
scope, and doesn't have much in common with the "enter a new 
parent-nested scope" kind that the scope mechanism of Parser uses.
Also, name lookup in Sema currently relies on 'CurContext' so the 
ActOnCXXEnterDeclaratorScope is useful to let it know that the current 
decl context should change.

Do you have an alternative suggestion in mind ?
Like, eliminating Sema's CurContext and putting that kind of information 
into Parser's Scope ?

> +  /// [C++]   template-id
> +  ///
> +  bool isTokenUnqualifiedId() const {
>
> Please put a [TODO] or [FIXME] on the template-id production.
>   

Ok.

> +  /// AnnotateToken - If the current token position is on a typename (possibly
> +  /// qualified in C++) or a C++ scope specifier not followed by a typename,
> +  /// AnnotateToken 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 AnnotateToken();
>
> I think the name of this routine could be better, for two reasons.
> First, it doesn't say what the annotations are doing---resolving
> qualified-ids and such. Second, I'd like there to be a "Maybe" or a
> "Try" at the beginning, to say that it might change the token stream
> (or it might not).
>   

TryAnnotateTypeOrScopeToken ?

-Argiris