[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
Fariborz Jahanian
fjahanian at apple.com
Tue Jul 21 15:40:22 PDT 2009
I think I took care of all your feedback in http://llvm.org/viewvc/llvm-project?view=rev&revision=75849
Let me know if I missed something.
- Thanks, Fariborz
On Jul 20, 2009, at 5:29 PM, Douglas Gregor wrote:
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090721/d873edab/attachment.html>
More information about the cfe-commits
mailing list