Patch for C++ namespaces

Hi,

The attached patch implements parsing of C++ namespaces:

-Added NamespaceDecl for the AST
-Added NamespaceScope flag in the Scope class
-Added checks for name clashes between namespaces and tag/normal declarations.

Note that proper name lookup for namespaces is not implemented, (i.e) :

namespace A {
  int x;
}

namespace A {
  void f() { x=0; }
}

I'll add it in a separate patch.

-Argiris

cxx-namespaces.patch (24.6 KB)

The attached patch implements parsing of C++ namespaces:

woo! This is great.

-Added NamespaceDecl for the AST
-Added NamespaceScope flag in the Scope class
-Added checks for name clashes between namespaces and tag/normal declarations.

Ok, a few questions below.

Note that proper name lookup for namespaces is not implemented, (i.e) :

namespace A {
int x;
}

namespace A {
void f() { x=0; }
}

I'll add it in a separate patch.

Sounds good. I'm all for incremental patches :slight_smile:

Some questions:

What is Scope::NamespaceScope needed for? It controls the setting of NamespaceParent, but I don't see any uses of it. Doesn't the notion of context subsume this?

+ // For extended namespace definitions:
+ //
+ // namespace A { int x; }
+ // namespace A { int y; }
+ //
+ // there will be one NamespaceDecl for each declaration.
+ // NextDeclarator points to the next extended declaration.

I'm a bit concerned about this. Particularly when DeclContext eventually contains a list of all the decls corresponding to its context, I would expect one context for 'A' and that context should eventually have both x and y in it. This example is a bit similar to "extern int x; int x;" which is two decls but are merged to only refer to one. Is there a reason to keep these namespaces as two separate namespaces?

-Chris

Chris Lattner wrote:

What is Scope::NamespaceScope needed for? It controls the setting of NamespaceParent, but I don't see any uses of it.

I was going to use it later but I could use "getFnParent()->getParent()" for this. Shall I remove it ?

  Doesn't the notion of context subsume this?

Note that there's a subtle difference between Scope and DeclContext, i.e:

namespace A {
  class C {
    void f();
  }
}

void A::C::f() {
  int x;
}

For 'x', the Scope chain would be
TU scope -> A::C::f() method -> x

while the DeclContext chain would be
TU -> A namespace -> C class -> f() method -> x

This is how I see it, does this sound reasonable ?

+ // For extended namespace definitions:
+ //
+ // namespace A { int x; }
+ // namespace A { int y; }
+ //
+ // there will be one NamespaceDecl for each declaration.
+ // NextDeclarator points to the next extended declaration.

I'm a bit concerned about this. Particularly when DeclContext eventually contains a list of all the decls corresponding to its context, I would expect one context for 'A' and that context should eventually have both x and y in it. This example is a bit similar to "extern int x; int x;" which is two decls but are merged to only refer to one. Is there a reason to keep these namespaces as two separate namespaces?

Actually it works similar to your example:

extern int x;
int x;

For the above, there are two decls created and reported at ASTConsumer::HandleTopLevelDecl, but they are merged and x refers to the second.

For:
namespace A { int x; }
namespace A { int y; }

There are two NamespaceDecls created and reported at ASTConsumer::HandleTopLevelDecl, but 'A' refers only to the first one (the 'original' namespace).
And while there's no list of decls associated with NamespaceDecls yet, the intention is that the 'original namespace' will contain the list (y will be associated with the first NamespaceDecl).

By reporting 2 namespace decls, the AST maps to the source code better, and a refactoring tool would be able to do (for example) "rename all 'A' namespaces" by traversing the AST.

If this sound reasonable, how about adding a OriginalNamespace member to NamespaceDecl so that extended namespace definition decls point to the 'original' one ?

-Argiris

Chris Lattner wrote:

What is Scope::NamespaceScope needed for? It controls the setting of NamespaceParent, but I don't see any uses of it.

I was going to use it later but I could use "getFnParent()->getParent()" for this. Shall I remove it ?

Yes please.

Doesn't the notion of context subsume this?

Note that there's a subtle difference between Scope and DeclContext, i.e:

namespace A {
class C {
  void f();
}

void A::C::f() {
int x;
}

For 'x', the Scope chain would be
TU scope -> A::C::f() method -> x

while the DeclContext chain would be
TU -> A namespace -> C class -> f() method -> x

This is how I see it, does this sound reasonable ?

Yep, this makes perfect sense to me.

+ // For extended namespace definitions:
+ //
+ // namespace A { int x; }
+ // namespace A { int y; }
+ //
+ // there will be one NamespaceDecl for each declaration.
+ // NextDeclarator points to the next extended declaration.

I'm a bit concerned about this. Particularly when DeclContext eventually contains a list of all the decls corresponding to its context, I would expect one context for 'A' and that context should eventually have both x and y in it. This example is a bit similar to "extern int x; int x;" which is two decls but are merged to only refer to one. Is there a reason to keep these namespaces as two separate namespaces?

Actually it works similar to your example:

extern int x;
int x;

For the above, there are two decls created and reported at ASTConsumer::HandleTopLevelDecl, but they are merged and x refers to the second.

For:
namespace A { int x; }
namespace A { int y; }

There are two NamespaceDecls created and reported at ASTConsumer::HandleTopLevelDecl, but 'A' refers only to the first one (the 'original' namespace).
And while there's no list of decls associated with NamespaceDecls yet, the intention is that the 'original namespace' will contain the list (y will be associated with the first NamespaceDecl).

Ok.

By reporting 2 namespace decls, the AST maps to the source code better, and a refactoring tool would be able to do (for example) "rename all 'A' namespaces" by traversing the AST.

If this sound reasonable, how about adding a OriginalNamespace member to NamespaceDecl so that extended namespace definition decls point to the 'original' one?

I think we should tackle this in a more systematic way. All "merging" of decls should be treated the same. It really would be nice to be able to walk all declarations of some decl.

The best way to implement this (in terms of space efficiency) is an on-the-side DenseMap<Decl*,Decl*> which would implement a linked list of decls. Then each decl can just have one bit that says whether it has an entry in the map (i.e. whether there is a 'next'). This can be hidden around behind an accessor, just like the attribute stuff is. Does this seem reasonable?

-Chris

Chris Lattner wrote:

For:
namespace A { int x; }
namespace A { int y; }

There are two NamespaceDecls created and reported at ASTConsumer::HandleTopLevelDecl, but 'A' refers only to the first one (the 'original' namespace).
And while there's no list of decls associated with NamespaceDecls yet, the intention is that the 'original namespace' will contain the list (y will be associated with the first NamespaceDecl).

Ok.

By reporting 2 namespace decls, the AST maps to the source code better, and a refactoring tool would be able to do (for example) "rename all 'A' namespaces" by traversing the AST.

If this sound reasonable, how about adding a OriginalNamespace member to NamespaceDecl so that extended namespace definition decls point to the 'original' one?

I think we should tackle this in a more systematic way. All "merging" of decls should be treated the same. It really would be nice to be able to walk all declarations of some decl.

A unified way to walk all 'merged' declaration would be great. How about I commit namespaces as it is (minus NamespaceScope), so that a way to deal generally with merged decls would
consider NamespaceDecls too.

The best way to implement this (in terms of space efficiency) is an on-the-side DenseMap<Decl*,Decl*> which would implement a linked list of decls. Then each decl can just have one bit that says whether it has an entry in the map (i.e. whether there is a 'next'). This can be hidden around behind an accessor, just like the attribute stuff is. Does this seem reasonable?

That's a good idea!

-Argiris

Makes sense to me! Note that Doug's recent patch proposes a way to handle merging, but it is (currently) specific to functiondecls.

-Chris

I looked at Argiris's namespace patch, and we took exactly the same
approach to handling redeclarations: keep the original declaration,
and attach the new declaration onto a chain of previous declarations
rather than pushing it into scope.

Once the DenseMap<Decl*, Decl*> for previous declarations is
available, both functions and namespaces can easily use it (and other
entities that can be redeclared, of course).

  - Doug