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

[Douglas Gregor]

Hi Piotr,

On Dec 26, 2008, at 8:01 PM, Piotr Rak wrote:
> I have played bit with clang, and implemented parsing of C++ using- 
> directive.

Great!

>
> I would like to work on Sema support for it too, but I'm still
> unfamiliar with project,
> and not really sure if I am going in *right* direction. Hints and
> comments are highly welcome :)
> Attached contains things I have done so far. Before I will continue
> adding AST and Sema support,
> I would like to explain what in am planing to do, because I might be
> missing something.

Okay, I'll have some comments on your patch below.

>  Using directive basically fits concept of ScopedDecl, but not really
> well NamedDecl concept.

There are other kinds of entities that are ScopedDecls/NamedDecls but  
don't actually have names.

>
> It is not really clear, how it should be represented. And ofcourse
> lookup will have to deal with it
> in special way.

Definitely.

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

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

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

>
> There are few possible options to deal with it not increasing size of
> DeclContext:
> - Add this member to each concrete ScopedDecl in which it may appear,
> or even new base class and MI
> - (Ab)use DeclContext.LookupPtr structure and associate this list with
> special name,
>  (same as used by UsingDirectiveDecl's?)
> - any others?
>
> First is probably more clean, but IMO using directives are not so
> often, and it would introduce some
> memory overhead for Decl's that don't use it.

I agree.

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

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

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

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

>
> I have also attached 'make report' results, since I have noticed
> CodeGenObjC/encode-test.m failure,
> not introduced by my change. I'am using gentoo gcc-4.3.2 x86 linux.
> so it might be compiler issue.

It's not failing on my Mac, so we'll have to have someone familiar  
with Objective C (and who has a Linux machine!) take a look. Thanks  
for pointing this out!

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.

+/// 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 (!).

+// Defined out-of-line here because of dependecy on AttributeList
+Action::DeclTy *Action::ActOnUsingDirective(Scope *CurScope,
+                                            SourceLocation UsingLoc,
+                                            SourceLocation  
NamespaceLoc,
+                                            const CXXScopeSpec &SS,
+                                            DeclTy *Namespace,
+                                            AttributeList *AttrList) {
+
+  // FIXME: Parser seems to assume that Action::ActOn* takes  
ownership over
+  // passed AttributeList, however other actions don't free it, is it
+  // temporary state or bug?
+  delete AttrList;
+  return 0;
+}

We're still working on the ownership issues with the Parse-Sema  
interaction (with Sebastian Redl leading the charge), so this is a  
good FIXME to leave in place to remind us to deal with these issues.

+  // FIXME: ConsiderNamespaceOnly parameter is added temporalily
+  // we will need more better way to specify lookup criterions for  
things
+  // like template specializations, explicit template instatatiation  
etc.
+
    /// More parsing and symbol table subroutines...
    Decl *LookupDecl(DeclarationName Name, unsigned NSI, Scope *S,
                     const DeclContext *LookupCtx = 0,
                     bool enableLazyBuiltinCreation = true,
-                  bool LookInParent = true);
+                   bool LookInParent = true,
+                   bool NamespaceNameOnly = false);

We are getting quite a few parameters here :) That's not a problem you  
need to worry about fixing, though. We'll think about how best to deal  
with them (since there are more parameters coming).

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.)

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