Attempt at CXX Namespaces

I've gotten serious and have written some code.

Following the clang web page's advice, I've tried to start implementing c++ namespaces.
Currently, I'm trying to figure out how to add namespace members to my NamespaceDecl.

I'm very new to clang and learning as I go. Comments/Suggestions welcome.

Kevin Tew

cxxnamespaces.diff (5.29 KB)

I've gotten serious and have written some code.

Following the clang web page's advice, I've tried to start implementing c++ namespaces.
Currently, I'm trying to figure out how to add namespace members to my NamespaceDecl.

I'm very new to clang and learning as I go. Comments/Suggestions welcome.

Great! First comments: please make sure your code fits into 80 columns. Second, make sure
comments are up to date: this is not a recorddecl :slight_smile:

+/// RecordDecl - Represents a cxx namespace . For example:
+//. namespace X { };
+/// This decl will be marked invalid if *any* members are invalid.
+///
+class NamespaceDecl : public RecordDecl {

Why is this class deriving from RecordDecl? Namespaces "are not" records, so there shouldn't be an inheritance relation here.

+
+public:
+ NamespaceDecl(Kind DK, SourceLocation L, IdentifierInfo *Id, ScopedDecl*PrevDecl)

In this ctor, I don't think there is any reason to pass in a 'Kind'. The kind of NamespaceDecl is always Namespace.

Finally, please send patches as an attachment with .patch suffix. This avoids problems with some mailers.

Thanks for working on this!

-Chris

Hmm, not a bad start. A couple of comments from reading your patch.
First, make sure you don't go over 80 characters per line. Second, I
don't think that making NamespaceDecl a subclass of RecordDecl really
makes sense: a namespace doesn't have members in the same sense that a
class or struct does. Maybe make it a direct subclass of ScopedDecl?
Also, a few more tests might be appropriate.

Doesn't necessarily need to be done in this patch, but it would be
nice if the places that print out namespaced identifiers (like dump()
and related) would print out the associated namespace, so that things
don't get so confusing.

Also, here's a testcase which I'm not sure your patch handles:
namespace a { double i; }
int i;
namespace a {double* j = &i;}
should not print a warning with -pedantic.

I can't really come up with any other tests that have to do with
namespaces in particular... namespaces are pretty simple without
"using" and the :: operator.

-Eli

I've tried to incorporate both Chris and your suggestions.

Kevin
Eli Friedman wrote:

cxxnamespaces_try2.patch (7.52 KB)

I don't believe namespace are a "ScopedDecl".

For now, I suggest NamespaceDecl inherits from Decl (like LinkageSpecDecl).

snaroff

I've tried to incorporate both Chris and your suggestions.

Okay, looking better.

One more thing I spotted: you can get rid of the FIXMEs in
ParseDeclCXX.cpp, since you're implementing them.

> Also, here's a testcase which I'm not sure your patch handles:
> namespace a { double i; }
> int i;
> namespace a {double* j = &i;}
> should not print a warning with -pedantic.

I don't think you checked this properly... try adding -pedantic to the
command line for your testcase. (clang unfortunately has to be
extremely loose about pointer conversions in general.)

Or just try this simpler testcase:
namespace a {double i;}
namespace a {double* j = &i;}

-Eli

I think we can tighten this up in C++ mode, right?

Also, Steve's right, namespaces should not be ScopedDecls.

-Chris

Chris Lattner wrote:-

Yeah, we probably should... gcc warns, so it shouldn't be an issue.
We should be careful not to warn by default about implicit
signed/unsigned conversion, though; people are likely to get annoyed
at a compiler warning that they're passing an int16_t* to a function
expecting a uint16_t*.

-Eli

I'm puzzled at how to go about adding child decls too a namespace.
I've picked up, that the parser shouldn't use AST types and only make Actions.ActOn... calls.

Currently I'm thinking something like this:

llvm::SmallVector<DeclTy*, 16> NamespaceChildren;
while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
  NamespaceChildren.push_back(ParseExternalDeclaration());
}
Actions.ActOnNamespaceMembers(&NamespaceChildren[0], NamespaceChildren.size());

or something like this:

while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
  Actions.ActOnNamespaceMember(ParseExternalDeclaration());
}

Am I on the right track?

Kevin

cxxnamespaces_try3.patch (7.72 KB)

I'm puzzled at how to go about adding child decls too a namespace.

Ok.

I've picked up, that the parser shouldn't use AST types and only make Actions.ActOn... calls.

Yep!

Currently I'm thinking something like this:

llvm::SmallVector<DeclTy*, 16> NamespaceChildren;
while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
NamespaceChildren.push_back(ParseExternalDeclaration());
}
Actions.ActOnNamespaceMembers(&NamespaceChildren[0], NamespaceChildren.size());

or something like this:

while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
Actions.ActOnNamespaceMember(ParseExternalDeclaration());
}

Am I on the right track?

I'd suggest calling one action when a namespace is started and one when finished (like you have). The former create a Namespacedecl object and push it as the current context. All decls parsed after that (and actions fired as a result of them) would cause the decls to get added to the current context (the namespace). When the namespace is closed, the current context is finished and removed.

This is somewhat like scope processing. Does this make sense?

-Chris