[cfe-dev] [PATCH]: Sema support for C++ classes

[Chris Lattner]

On Jun 25, 2008, at 9:49 AM, Argiris Kirtzidis 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?



+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 @@
+
+Action::ExprResult Sema::ActOnCXXThis(SourceLocation ThisLoc) {
+  /// C++ 9.3.2: In the body of a non-static member function, the  
keyword this is

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


+Sema::DeclTy *
+Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS,  
Declarator &D,


+  assert(II && "No identifier ?");

does this abort on unnamed bitfields like "int : 4;" ?



+  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);


Otherwise, this looks great Argiris!

-Chris