Hi,
The attached patch adds the missing Sema support for parsing C++ classes.
About references to instance fields:
+ // FIXME: Use DeclRefExpr or a new Expr for a direct CXXField reference.
+ ExprResult ThisExpr = ActOnCXXThis(Loc);
+ return new MemberExpr(static_cast<Expr*>(ThisExpr.Val),
+ true, FD, Loc, FD->getType());
I'll fix this once Sema support is in place.
-Argiris
cxx-sema-2.patch (28.1 KB)
Ok! One meta question: the PushDeclContext/pop context logic has to keep the CurMethodDecl members up to date, and are somewhat complex. Would it be better to change direct accesses to CurMethodDecl to call getCurMethodDecl(), which would just look at the current context?
+DIAG(err_member_initialization, ERROR,
+ "'%0' can be initialized only if it is a static const integral data member")
How about "can only be initialized if..."
+DIAG(err_invalid_this_at_top_level, ERROR,
+ "invalid use of 'this' at top level")
How about "outside of a member function"? It would also subsume err_invalid_this_in_non_member_func
Are methods in C++ officially known as "member functions" or "methods"?
+++ lib/Sema/Sema.h (working copy)
+ /// CXXFieldCollector - Used to keep track of CXXFieldDecls during parsing of
+ /// C++ classes.
+ class {
Instead of nesting this inside the sema class, please pull this out to its own header (or put in SemaUtil.h). The sema class is already very large, which means it is hard to understand "at a glance". Also, it is probably best for Sema to have a pointer (e.g. an OwningPtr) to the instance instead of having it inline. This allows it to be lazily created and helps keep sizeof(Sema) reasonable.
+ llvm::SmallVector<CXXFieldDecl*, 32> Fields;
+ llvm::SmallVector<size_t, 4> FieldCount;
Please comment what these are and what invariants hold.
+ /// getNumField - The number of fields added to the currently parsed class.
+ size_t getNumField() const { return FieldCount.back(); }
"getNumFields"?
+ /// StartClass - Called by Sema::ActOnFinishCXXClassDef.
+ void FinishClass() {
plz update comment.
+ assert(isa<CXXMethodDecl>(FD) || CurFunctionDecl == 0
+ && "Confused parsing.");
You need parens here.
+ if (getLangOptions().CPlusPlus)
+ New = CXXRecordDecl::Create(Context, Kind, CurContext, Loc, Name, 0);
+ else
+ New = RecordDecl::Create(Context, Kind, CurContext, Loc, Name, 0);
Is it hopeless to use the "light weight recorddecl" for "simple enough structs" even in C++?
+ if (getLangOptions().CPlusPlus) {
+ NewFD = CXXFieldDecl::Create(Context, cast<CXXRecordDecl>(CurContext),
+ Loc, II, T, BitWidth);
+ PushOnScopeChains(NewFD, S);
+ }
+ else
+ NewFD = FieldDecl::Create(Context, Loc, II, T, BitWidth);
likewise.
+++ lib/Sema/SemaExprCXX.cpp (working copy)
@@ -49,3 +49,21 @@
Chris Lattner wrote:
Hi,
The attached patch adds the missing Sema support for parsing C++ classes.
About references to instance fields:
+ // FIXME: Use DeclRefExpr or a new Expr for a direct CXXField reference.
+ ExprResult ThisExpr = ActOnCXXThis(Loc);
+ return new MemberExpr(static_cast<Expr*>(ThisExpr.Val),
+ true, FD, Loc, FD->getType());I'll fix this once Sema support is in place.
Ok! One meta question: the PushDeclContext/pop context logic has to keep the CurMethodDecl members up to date, and are somewhat complex. Would it be better to change direct accesses to CurMethodDecl to call getCurMethodDecl(), which would just look at the current context?
Sounds great! Shall I make a separate commit for this ?
+DIAG(err_member_initialization, ERROR,
+ "'%0' can be initialized only if it is a static const integral data member")How about "can only be initialized if..."
+DIAG(err_invalid_this_at_top_level, ERROR,
+ "invalid use of 'this' at top level")How about "outside of a member function"? It would also subsume err_invalid_this_in_non_member_func
Ok.
Are methods in C++ officially known as "member functions" or "methods"?
The standard uses "member functions".
+++ lib/Sema/Sema.h (working copy)
+ /// CXXFieldCollector - Used to keep track of CXXFieldDecls during parsing of
+ /// C++ classes.
+ class {Instead of nesting this inside the sema class, please pull this out to its own header (or put in SemaUtil.h). The sema class is already very large, which means it is hard to understand "at a glance". Also, it is probably best for Sema to have a pointer (e.g. an OwningPtr) to the instance instead of having it inline. This allows it to be lazily created and helps keep sizeof(Sema) reasonable.
+ llvm::SmallVector<CXXFieldDecl*, 32> Fields;
+ llvm::SmallVector<size_t, 4> FieldCount;Please comment what these are and what invariants hold.
+ /// getNumField - The number of fields added to the currently parsed class.
+ size_t getNumField() const { return FieldCount.back(); }"getNumFields"?
+ /// StartClass - Called by Sema::ActOnFinishCXXClassDef.
+ void FinishClass() {plz update comment.
+ assert(isa<CXXMethodDecl>(FD) || CurFunctionDecl == 0
+ && "Confused parsing.");You need parens here.
Ok to all.
+ if (getLangOptions().CPlusPlus)
+ New = CXXRecordDecl::Create(Context, Kind, CurContext, Loc, Name, 0);
+ else
+ New = RecordDecl::Create(Context, Kind, CurContext, Loc, Name, 0);Is it hopeless to use the "light weight recorddecl" for "simple enough structs" even in C++?
+ if (getLangOptions().CPlusPlus) {
+ NewFD = CXXFieldDecl::Create(Context, cast<CXXRecordDecl>(CurContext),
+ Loc, II, T, BitWidth);
+ PushOnScopeChains(NewFD, S);
+ }
+ else
+ NewFD = FieldDecl::Create(Context, Loc, II, T, BitWidth);likewise.
Using plain FieldDecls for simple structs should be easy, I'm not sure about CXX/RecordDecl.
Can we add this as is so we have the functionality and tests in, and later look for ways to optimize it ?
+++ lib/Sema/SemaExprCXX.cpp (working copy)
@@ -49,3 +49,21 @@
+
+Action::ExprResult Sema::ActOnCXXThis(SourceLocation ThisLoc) {
+ /// C++ 9.3.2: In the body of a non-static member function, the keyword this is80 col violation
+++ lib/Sema/SemaExpr.cpp (working copy)
+ if (MD->isStatic())
+ // "invalid use of member 'x' in static member function"
+ return Diag(Loc, diag::err_invalid_member_use_in_static_method, FD->getName());
+ if (cast<CXXRecordDecl>(MD->getParent()) != FD->getParent())likewise.
===================================================================
--- lib/Sema/SemaDeclCXX.cpp (revision 52718)
+++ lib/Sema/SemaDeclCXX.cpp (working copy)in Sema::ActOnStartCXXClassDef and:
+void Sema::ActOnFinishCXXClassDef(DeclTy *D,SourceLocation RBrace) {
+ Decl *Dcl = static_cast<Decl *>(D);
+ CXXRecordDecl *RD = dyn_cast_or_null<CXXRecordDecl>(Dcl);
+ assert(RD && "Invalid parameter, expected CXXRecordDecl");
+ CXXFieldCollector.FinishClass();
+ PopDeclContext();
+}Instead of using dyn_cast_or_null + assert(not null), just use cast<>
Ok to all.
+Sema::DeclTy *
+Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,+ assert(II && "No identifier ?");
does this abort on unnamed bitfields like "int : 4;" ?
This requires changes to the parser too. Can we fix it after the patch goes in so I can make one "unified" Parser+Sema+test commit, specific to this case ?
+ CXXClassMemberWrapper MW(Member);
+ MW.setAccess(AS);
+What does this do? I had to look this up, it is somewhat strange. At the least, how about:
// What this does and why we can't just call Member->setAccess(..)
CXXClassMemberWrapper(Member).setAccess(AS);
Ok.
-Argiris
I'd suggest figuring out whatever parser changes are needed, commit
that fix, then integrate the Sema changes into the next revision of
this patch; the parser fix looks mostly independent.
On a somewhat related note, the comment "// Attributes are only
allowed on the second declarator." in ParseCXXClassMemberDeclaration
is wrong, I think; g++ accepts "class C {int a
__attribute((mode(HI)));};".
-Eli
Hi Eli,
Eli Friedman wrote:
+ assert(II && "No identifier ?");
does this abort on unnamed bitfields like "int : 4;" ?
This requires changes to the parser too. Can we fix it after the patch
goes in so I can make one "unified" Parser+Sema+test commit, specific to
this case ?
I'd suggest figuring out whatever parser changes are needed, commit
that fix, then integrate the Sema changes into the next revision of
this patch; the parser fix looks mostly independent.
Sounds reasonable.
On a somewhat related note, the comment "// Attributes are only
allowed on the second declarator." in ParseCXXClassMemberDeclaration
is wrong, I think; g++ accepts "class C {int a
__attribute((mode(HI)));};".
I copied the attribute parsing from ParseStructDeclaration.
The attribute you mention is parsed some lines before with:
// If attributes exist after the declarator, parse them.
if (Tok.is(tok::kw___attribute))
DeclaratorInfo.AddAttributes(ParseAttributes());
Nevertheless, that comment seems a bit misleading to me too..
-Argiris
Ah, mmm... the construct that it's talking about is something like
"struct S {int a, __attribute((aligned(8))) b;};". (For some strange
reason, gcc only accepts this particular snippet in C++ mode.) The
comment could definitely be made clearer, though.
-Eli
I'll fix this once Sema support is in place.
Ok! One meta question: the PushDeclContext/pop context logic has to keep the CurMethodDecl members up to date, and are somewhat complex. Would it be better to change direct accesses to CurMethodDecl to call getCurMethodDecl(), which would just look at the current context?
Sounds great! Shall I make a separate commit for this ?
Yes, please do it as a separate patch, thanks!
Are methods in C++ officially known as "member functions" or "methods"?
The standard uses "member functions".
Ok!
+ else
+ NewFD = FieldDecl::Create(Context, Loc, II, T, BitWidth);likewise.
Using plain FieldDecls for simple structs should be easy, I'm not sure about CXX/RecordDecl.
Can we add this as is so we have the functionality and tests in, and later look for ways to optimize it ?
I'm fine with making it a fixme for now, and addressing it later as functionality matures.
+Sema::DeclTy *
+Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,+ assert(II && "No identifier ?");
does this abort on unnamed bitfields like "int : 4;" ?
This requires changes to the parser too. Can we fix it after the patch goes in so I can make one "unified" Parser+Sema+test commit, specific to this case ?
Yep, this patch is definite progress forward. I'm happy with forward progress (even if it doesn't solve all problems) as long as it doesn't regress anything that currently works.
Thanks Argiris!
-Chris
Replaced CurFunctionDecl and CurMethodDecl:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080623/006281.html
Fixed parsing of unnamed bitfields:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080623/006283.html
I attached a new patch for Sema support, with the suggestions.
-Argiris
cxx-sema-3.patch (29.4 KB)
Argiris Kirtzidis wrote:
I attached a new patch for Sema support, with the suggestions.
Please excuse this "leftover" part that I missed to clear out:
Index: lib/Sema/Sema.cpp
+ // FIXME: Use DeclRefExpr or a new Expr for a direct CXXField reference.
+ ExprResult ThisExpr = ActOnCXXThis(Loc);
Make this line "ExprResult ThisExpr =
ActOnCXXThis(SourceLocation());". (This isn't really the right fix,
but it's a bit better than what's there.)
Otherwise, I think looks fine.
-Eli
Looks great to me!
-Chris
Eli Friedman wrote: