[cfe-dev] C++ using-directive parsing

[Piotr Rak]

Thanks for review, comments and anwsers. :)

2008/12/27 Douglas Gregor <dgregor at apple.com>:
> Hi Piotr,
>
> On Dec 26, 2008, at 8:01 PM, Piotr Rak wrote:
>> My idea is to represent it as ScopedDecl and introduce special name
>> (DeclarationNameExtra instance),
>> shared by all UsingDirectiveDecl's. Those would be added to
>> DeclContext in which they appear
>> (in case of namespace only to original namespace). Does it sound sane?
>
> That's interesting. It'll have to be an overloaded name, just like function
> names can be overloaded, but it does make it easy to fit into the existing
> name lookup mechanisms (in DeclContext). That said, it seems slightly weird
> to have an actual name for using directives... perhaps we can hide the fact
> that they have a name, encapsulating it inside the logic of DeclContext,
> DeclarationName, and Sema?
>
That was idea, I think it is possible.
>>
>> On lookup, list of all namespaces, both introduced directly and
>> indirectly by using directive
>> would be build.
>
> My initial reaction is that keeping track of namespaces introduced
> indirectly will add more memory and complexity overhead than it will save in
> time. But, let's explore this further.

Ok.

>> I belive it would be wise to cache it, which would
>> introduce another member to DeclContext.
>> DeclContext should be pretty common citizen in AST, well maybe not in
>> headers, so it could be some memory
>> overhead, especially for C. Or Maybe I just worry to early.
>
> It's good to keep DeclContext small, if we can, because it is used in many
> places. Interestingly, namespace directives can also be used within blocks,
> which aren't DeclContexts. I wonder if that will change, or if we'll instead
> want to put something into the Scope for blocks.

I was not aware of this issue.
>From my point of view it would be easier handle it consistenly in DeclContext,
however didn't looked at this code yet. I'll take a look how big
change it would be.

>>
>> Second feels like ugly hack, but it doesn't share this disadvantage
>> with first, but requires some overhead
>> (hashtable lookup). Initially I would be in favour of implementing of
>> second option. There is also FIXME,
>> that says about need of replacing structure held by LookupPtr, so it
>> could be smartly abstracted later.
>
> I like this second option, and I think that if we abstract it well---e.g.,
> by providing special methods inside DeclContext to lookup the list of using
> directives rather than having the client create a special DeclarationName
> each time---and document the slightly sneaky use of DeclarationName, it'll
> be clean.

True.

>>
>> Another minor issue is keeping list of used namespaces up to date,
>> which probably will require some
>> knowledge from namespace about its 'users'. Here I would like to
>> introduce distinction between original-namespace,
>> and extended-namespace. This not really required, but seems to be good
>> idea, because original namespace will
>> have to keep bit more information than extended namespace,
>
> I think this complexity comes from trying to keep the list of namespaces
> indirectly searched, but I don't think that's an important case to optimize
> for. It only takes a single hash-table lookup into each DeclContext to find
> the using directives at each level, and I'm guessing that it's rare for
> there to be more than two or three levels of indirection to deal with in
> real programs. Let's just start by keeping track of the direct using
> directives; it'll save space and keep the complexity down. If we find that
> the depth of the using-directive search is getting large, we'll consider
> doing something more involved.

Ok, I leave it for now, should not be to hard to add later.

>> later also
>> things like C++0x
>> inline namespace/gnu strong-using extension. This could be done same
>> *transparent* way, that was suggested
>> by Mr. Douglas Gregor some time ago (strangely couldn't find pointers
>> to this message now).
>
> I'm still working on that one. Inline namespace should be pretty easy once
> I've finished that work into transparent DeclContexts.

Great!

> (Oh, and I'm just "Doug" <g>)

I'll try to remember. :)

> Some comments on your patch:
>
> @@ -225,6 +227,9 @@ Parser::DeclTy *Parser::ParseDeclaration(unsigned
> Context) {
>     return ParseTemplateDeclaration(Context);
>   case tok::kw_namespace:
>     return ParseNamespace(Context);
> +  case tok::kw_using:
> +    if (isCXXUsingDirective()) return ParseUsingDirective(Context);
> +    else return ParseUsingDeclaration(Context); //FIXME: it is just stub
>   default:
>     return ParseSimpleDeclaration(Context);
>
> Instead of having the isCXXUsingDirective---which requires lookahead---could
> you just have a ParseUsingDirectiveOrDeclaration function that eats the
> "using" and then decides whether it goes on to parse a using directive or
> using declaration? It would be slightly more efficient.

Yes, I initially had ParseUsingDirectiveOrDeclaration during evolution.
Later had short, failed attempt implementing c++0x:
 using-directive:
      attribute-specifier[opt] using namespace ::[opt]
nested-name-specifier[opt] namespace-name ;

requiring tentative parsing, and forgot change it back. Fixed now in
attached version.

> +/// ParseNamespaceName - Parse c++ namespace-name, returns null and issues
> +/// diagnostics if current token is not identifier or it is not namespace
> +/// name. Otherwise returns found namespace declaration.
> +Parser::DeclTy *Parser::ParseNamespaceName(const CXXScopeSpec &SS) {
>
> I think ParseNamespaceName is trying to do more work than it should. It's
> calling isNamespaceName to check whether the identifier is a namespace,
> effectively performing some of the semantic analysis for using directives.
> However, in this case the parse won't parse differently dependent on whether
> a name is a namespace-name, class-name, or something else. So, I suggest
> just parsing the identifier as an identifier (in ParseUsingDirective) and
> then passing the identifier and its source location directly to
> Actions.ActOnUsingDirective. Then, semantic analysis can lookup the
> identifier in the scope of 'SS', complain if it's not a namespace-name, and
> do whatever other AST-building and semantic-analysis tasks it needs to. That
> would probably mean that we would no longer need isNamespaceName (!).

Done, indeed, fixes -parse-noop.

> In the new test (cxx-using-directive.cpp), I think we also need to test for
> something where '::' prefixes the nested-name-specifier, e.g.,
>
>        using namespace ::D::B;
>
> (I tried it, and it works fine, but this will make sure it continues to
> work.)
Done, and few more.

> I think that with the change in the way ActOnUsingDirective works (and
> removal of isNamespaceName, if that all works out), this will be ready to go
> in. Thanks for working on this!
>
>        - Doug
>

I made additional change, now ActOnUsingDirective is not called when
we fail parse CXXScopeSpec or identifier. Is this ok, or we should
create invalid decl in such cases?

Piotr
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Initial-Parser-support-for-C-using-directive_v2.patch
Type: text/x-patch
Size: 16996 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081228/4920acae/attachment.bin>