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

[Argiris Kirtzidis]

Chris Lattner wrote:
>
> 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?

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

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