[cfe-commits] r75849 - in /cfe/trunk: include/clang/AST/DeclCXX.h include/clang/Parse/Action.h lib/AST/DeclCXX.cpp lib/AST/DeclPrinter.cpp lib/Parse/ParseCXXInlineMethods.cpp lib/Parse/Parser.cpp lib/Sema/Sema.h lib/Sema/SemaDeclCXX.cpp
Douglas Gregor
dgregor at apple.com
Mon Jul 20 17:29:36 PDT 2009
On Jul 15, 2009, at 3:34 PM, Fariborz Jahanian wrote:
> Author: fjahanian
> Date: Wed Jul 15 17:34:08 2009
> New Revision: 75849
>
> URL: http://llvm.org/viewvc/llvm-project?rev=75849&view=rev
> Log:
> Added ASTs to destructor decl AST for default destruction of object's
> base/members.
Okay, some comments below.
> Modified:
> cfe/trunk/include/clang/AST/DeclCXX.h
> cfe/trunk/include/clang/Parse/Action.h
> cfe/trunk/lib/AST/DeclCXX.cpp
> cfe/trunk/lib/AST/DeclPrinter.cpp
> cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
> cfe/trunk/lib/Parse/Parser.cpp
> cfe/trunk/lib/Sema/Sema.h
> cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>
> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=75849&r1=75848&r2=75849&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
> +++ cfe/trunk/include/clang/AST/DeclCXX.h Wed Jul 15 17:34:08 2009
> @@ -895,7 +895,16 @@
> /// explicitly defaulted (i.e., defined with " = default") will have
> /// @c !Implicit && ImplicitlyDefined.
> bool ImplicitlyDefined : 1;
> -
> +
> + /// Support for base and member destruction.
> + /// BaseOrMemberDestructions - The arguments used to destruct the
> base
> + /// or member.
> + // FIXME. May want to use a new class as
> CXXBaseOrMemberInitializer has
> + // more info. than is needed. At the least,
> CXXBaseOrMemberInitializer need
> + // be renamed to something neutral.
> + CXXBaseOrMemberInitializer **BaseOrMemberDestructions;
> + unsigned NumBaseOrMemberDestructions;
CXXBaseOrMemberInitializer is the wrong abstraction for this,
especially because it contains the Args, NumArgs, and IdLoc fields
that we don't need. I'd suggest storing a uintptr_t that stores either
a RecordDecl* or a FieldDecl* (for bases and members, respectively),
and use the lower two bits to encode one of the three options: it's a
virtual base (stored as a RecordDecl*), it's a direct non-virtual base
(stored as a RecordDecl*), or it's a non-static data members (stored
as a FieldDecl*). That should be all of the information that clients
(such as CodeGen) need to perform destruction properly.
Also, I think it's important for the comment to note that we're
storing the destructions in the order they should occur (which is the
reverse of construction order).
> CXXDestructorDecl(CXXRecordDecl *RD, SourceLocation L,
> DeclarationName N, QualType T,
> bool isInline, bool isImplicitlyDeclared)
> @@ -928,6 +937,34 @@
> ImplicitlyDefined = ID;
> }
>
> + /// destr_iterator - Iterates through the member/base destruction
> list.
> + typedef CXXBaseOrMemberInitializer **destr_iterator;
> +
> + /// destr_const_iterator - Iterates through the member/base
> destruction list.
> + typedef CXXBaseOrMemberInitializer * const * destr_const_iterator;
> +
> + /// destr_begin() - Retrieve an iterator to the first destructed
> member/base.
> + destr_iterator destr_begin() { return
> BaseOrMemberDestructions; }
> + /// destr_begin() - Retrieve an iterator to the first destructed
> member/base.
> + destr_const_iterator destr_begin() const { return
> BaseOrMemberDestructions; }
> +
> + /// destr_end() - Retrieve an iterator past the last destructed
> member/base.
> + destr_iterator destr_end() {
> + return BaseOrMemberDestructions + NumBaseOrMemberDestructions;
> + }
> + /// destr_end() - Retrieve an iterator past the last destructed
> member/base.
> + destr_const_iterator destr_end() const {
> + return BaseOrMemberDestructions + NumBaseOrMemberDestructions;
> + }
> +
> + /// getNumArgs - Determine the number of arguments used to
> + /// destruct the member or base.
> + unsigned getNumBaseOrMemberDestructions() const {
> + return NumBaseOrMemberDestructions;
> + }
The comment for getNumBaseOrMemberDestructions() is wrong.
>
> + void setBaseOrMemberDestructions(ASTContext &C);
> +
The name of this method is confusing: we're not really "setting" the
base or member destructions, we're "computing" the bases and members
that will be implicitly destroyed. A comment would also help!
>
> // Implement isa/cast/dyncast/etc.
> static bool classof(const Decl *D) {
> return D->getKind() == CXXDestructor;
>
> Modified: cfe/trunk/include/clang/Parse/Action.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Action.h?rev=75849&r1=75848&r2=75849&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/include/clang/Parse/Action.h (original)
> +++ cfe/trunk/include/clang/Parse/Action.h Wed Jul 15 17:34:08 2009
> @@ -1233,7 +1233,7 @@
> MemInitTy **MemInits, unsigned
> NumMemInits){
> }
>
> - virtual void ActOnDefaultInitializers(DeclPtrTy ConstructorDecl) {}
> + virtual void ActOnDefaultCDtorInitializers(DeclPtrTy CDtorDecl) {}
> /// ActOnFinishCXXMemberSpecification - Invoked after all member
> declarators
> /// are parsed but *before* parsing of inline method definitions.
>
> Modified: cfe/trunk/lib/AST/DeclCXX.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclCXX.cpp?rev=75849&r1=75848&r2=75849&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/AST/DeclCXX.cpp (original)
> +++ cfe/trunk/lib/AST/DeclCXX.cpp Wed Jul 15 17:34:08 2009
> @@ -478,6 +478,50 @@
> }
>
> void
> +CXXDestructorDecl::setBaseOrMemberDestructions(ASTContext &C) {
> + CXXRecordDecl *ClassDecl = cast<CXXRecordDecl>(getDeclContext());
> + llvm::SmallVector<CXXBaseOrMemberInitializer*, 32> AllToDestruct;
> + for (CXXRecordDecl::base_class_iterator VBase = ClassDecl-
> >vbases_begin(),
> + E = ClassDecl->vbases_end(); VBase != E; ++VBase) {
> + CXXBaseOrMemberInitializer *Member =
> + new CXXBaseOrMemberInitializer(VBase->getType(), 0,
> 0,SourceLocation());
> + AllToDestruct.push_back(Member);
> + }
If the virtual base class has a trivial destructor, it shouldn't be in
the list, right?
> + for (CXXRecordDecl::base_class_iterator Base =
> + ClassDecl->bases_begin(),
> + E = ClassDecl->bases_end(); Base != E; ++Base) {
> + if (Base->isVirtual())
> + continue;
> + CXXBaseOrMemberInitializer *Member =
> + new CXXBaseOrMemberInitializer(Base->getType(), 0, 0,
> SourceLocation());
> + AllToDestruct.push_back(Member);
> + }
Same question: if the direct non-virtual base has a trivial
destructor, it shouldn't be in the list.
> + // non-static data members.
> + for (CXXRecordDecl::field_iterator Field = ClassDecl->field_begin
> (),
> + E = ClassDecl->field_end(); Field != E; ++Field) {
> + QualType FieldType = C.getCanonicalType((*Field)->getType());
> + while (const ArrayType *AT = C.getAsArrayType(FieldType))
> + FieldType = AT->getElementType();
> +
> + if (FieldType->getAsRecordType()) {
> + CXXBaseOrMemberInitializer *Member =
> + new CXXBaseOrMemberInitializer((*Field), 0, 0,
> SourceLocation());
> + AllToDestruct.push_back(Member);
> + }
Same question, but also: if the field is not of class type, then it
shouldn't be in the list of destructions.
> + }
> +
> + unsigned NumDestructions = AllToDestruct.size();
> + if (NumDestructions > 0) {
> + NumBaseOrMemberDestructions = NumDestructions;
> + BaseOrMemberDestructions =
> + new (C) CXXBaseOrMemberInitializer*[NumDestructions];
> + // Insert in reverse order.
> + for (int Idx = NumDestructions-1, i=0 ; Idx >= 0; --Idx)
> + BaseOrMemberDestructions[i++] = AllToDestruct[Idx];
> + }
> +}
> +
> +void
> CXXConstructorDecl::setBaseOrMemberInitializers(
> ASTContext &C,
> CXXBaseOrMemberInitializer
> **Initializers,
CXXDestructorDecl should have a Destroy method that deallocates the
BaseOrMemberDestructions pointer.
> Modified: cfe/trunk/lib/AST/DeclPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclPrinter.cpp?rev=75849&r1=75848&r2=75849&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/AST/DeclPrinter.cpp (original)
> +++ cfe/trunk/lib/AST/DeclPrinter.cpp Wed Jul 15 17:34:08 2009
> @@ -374,6 +374,36 @@
> }
> }
> }
> + else if (CXXDestructorDecl *DDecl = dyn_cast<CXXDestructorDecl>
> (D)) {
> + if (DDecl->getNumBaseOrMemberDestructions() > 0) {
> + // FIXME. This is strictly for visualization of
> destructor's AST for
> + // how base/members are destructed. It has no other validity.
> + assert (D->isThisDeclarationADefinition() && "Destructor
> with dtor-list");
> + Proto += " : ";
> + for (CXXDestructorDecl::destr_const_iterator B = DDecl-
> >destr_begin(),
> + E = DDecl->destr_end();
> + B != E; ++B) {
> + CXXBaseOrMemberInitializer * BMInitializer = (*B);
> + if (B != DDecl->destr_begin())
> + Proto += ", ";
> +
> + if (BMInitializer->isMemberInitializer()) {
> + FieldDecl *FD = BMInitializer->getMember();
> + Proto += "~";
> + Proto += FD->getNameAsString();
> + }
> + else // FIXME. skip dependent types for now.
> + if (const RecordType *RT =
> + BMInitializer->getBaseClass()->getAsRecordType()) {
> + const CXXRecordDecl *BaseDecl =
> + cast<CXXRecordDecl>(RT->getDecl());
> + Proto += "~";
> + Proto += BaseDecl->getNameAsString();
> + }
> + Proto += "()";
> + }
> + }
> + }
It's unfortunate that this unnecessarily breaks round-tripping of C++
code with -ast-print. Could you wrap the output of this in "/*" and
"*/"?
> else
> AFT->getResultType().getAsStringInternal(Proto, Policy);
> } else {
>
> Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp?rev=75849&r1=75848&r2=75849&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original)
> +++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Wed Jul 15
> 17:34:08 2009
> @@ -172,7 +172,7 @@
> if (Tok.is(tok::colon))
> ParseConstructorInitializer(LM.D);
> else
> - Actions.ActOnDefaultInitializers(LM.D);
> + Actions.ActOnDefaultCDtorInitializers(LM.D);
I find it a bit strange that ActOnDefaultCDtorInitializers gets called
for all member function definitions, not just constructors and (with
this change) destructors. More comments on this action below...
> // FIXME: What if ParseConstructorInitializer doesn't leave us
> with a '{'??
> ParseFunctionStatementBody(LM.D);
>
> Modified: cfe/trunk/lib/Parse/Parser.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/Parser.cpp?rev=75849&r1=75848&r2=75849&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Parse/Parser.cpp (original)
> +++ cfe/trunk/lib/Parse/Parser.cpp Wed Jul 15 17:34:08 2009
> @@ -666,7 +666,7 @@
> if (Tok.is(tok::colon))
> ParseConstructorInitializer(Res);
> else
> - Actions.ActOnDefaultInitializers(Res);
> + Actions.ActOnDefaultCDtorInitializers(Res);
>
> return ParseFunctionStatementBody(Res);
> }
>
> Modified: cfe/trunk/lib/Sema/Sema.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.h?rev=75849&r1=75848&r2=75849&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Sema/Sema.h (original)
> +++ cfe/trunk/lib/Sema/Sema.h Wed Jul 15 17:34:08 2009
> @@ -1468,7 +1468,7 @@
> SourceLocation
> MemberLoc,
> IdentifierInfo
> &Member,
> DeclPtrTy
> ImplDecl);
> - virtual void ActOnDefaultInitializers(DeclPtrTy ConstructorDecl);
> + virtual void ActOnDefaultCDtorInitializers(DeclPtrTy CDtorDecl);
> bool ConvertArgumentsForCall(CallExpr *Call, Expr *Fn,
> FunctionDecl *FDecl,
> const FunctionProtoType *Proto,
>
> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=75849&r1=75848&r2=75849&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Wed Jul 15 17:34:08 2009
> @@ -902,15 +902,18 @@
> }
> }
>
> -void Sema::ActOnDefaultInitializers(DeclPtrTy ConstructorDecl) {
> - if (!ConstructorDecl)
> +void Sema::ActOnDefaultCDtorInitializers(DeclPtrTy CDtorDecl) {
> + if (!CDtorDecl)
> return;
>
> if (CXXConstructorDecl *Constructor
> - = dyn_cast<CXXConstructorDecl>(ConstructorDecl.getAs<Decl>()))
> + = dyn_cast<CXXConstructorDecl>(CDtorDecl.getAs<Decl>()))
> Constructor->setBaseOrMemberInitializers(Context,
>
> (CXXBaseOrMemberInitializer **)0, 0);
> -
> + else
> + if (CXXDestructorDecl *Destructor
> + = dyn_cast<CXXDestructorDecl>(CDtorDecl.getAs<Decl>()))
> + Destructor->setBaseOrMemberDestructions(Context);
> }
I don't think that setBaseOrMemberDestructions should be called here.
Instead, setBaseOrMemberDestructions (with whatever new name it gets)
should be called from Sema::ActOnFinishFunctionBody, because
*semantically* these destructions occur at the end of the destructor,
and we don't need the parser to tell us when they happen. Another side
benefit of this behavior is that it will make sure that these
destructions are computed for destructors of class template
specializations, which isn't happening now.
- Doug
More information about the cfe-commits
mailing list