[cfe-commits] r76663 - 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/SemaDecl.cpp lib/Sema/SemaDeclCXX.cpp

[Douglas Gregor]

On Jul 21, 2009, at 3:36 PM, Fariborz Jahanian wrote:

> Author: fjahanian
> Date: Tue Jul 21 17:36:06 2009
> New Revision: 76663
>
> URL: http://llvm.org/viewvc/llvm-project?rev=76663&view=rev
> Log:
> Patch to accomodate Doug's comment on default
> destruction of base/members for each destructor AST.

Great, thanks! Some minor comments below.

> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=76663&r1=76662&r2=76663&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
> +++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Jul 21 17:36:06 2009
> @@ -901,10 +901,7 @@
>   /// 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;
> +  uintptr_t *BaseOrMemberDestructions;

Please document what the uintptr_t values actually mean.

> +  /// isVbaseToDestroy - returns true, if object is virtual base.
> +  bool isVbaseToDestroy(uintptr_t Vbase) const {
> +    return (Vbase & 0x1) != 0;
> +  }
> +  /// isDirectNonVBaseToDestroy - returns true, if object is direct  
> non-virtual
> +  /// base.
> +  bool isDirectNonVBaseToDestroy(uintptr_t DrctNonVbase) const {
> +    return (DrctNonVbase & 0x2) != 0;
> +  }
> +  /// isAnyBaseToDestroy - returns true, if object is any base  
> (virtual or
> +  /// direct non-virtual)
> +  bool isAnyBaseToDestroy(uintptr_t AnyBase) const {
> +    return (AnyBase & 0x3) != 0;
> +  }
> +  /// isMemberToDestroy - returns true if object is a non-static  
> data member.
> +  bool isMemberToDestroy(uintptr_t Member) const {
> +    return (Member & 0x3)  == 0;
> +  }
> +  /// getAnyBaseClassToDestroy - Get the type for the given base  
> class object.
> +  Type *getAnyBaseClassToDestroy(uintptr_t Base) const {
> +    if (isAnyBaseToDestroy(Base))
> +      return reinterpret_cast<Type*>(Base  & ~0x03);
> +    return 0;
> +  }
> +  /// getMemberToDestroy - Get the member for the given object.
> +  FieldDecl *getMemberToDestroy(uintptr_t Member) {
> +    if (isMemberToDestroy(Member))
> +      return reinterpret_cast<FieldDecl *>(Member);
> +    return 0;
> +  }
> +  /// getVbaseClassToDestroy - Get the virtual base.
> +  Type *getVbaseClassToDestroy(uintptr_t Vbase) const {
> +    if (isVbaseToDestroy(Vbase))
> +      return reinterpret_cast<Type*>(Vbase  & ~0x01);
> +    return 0;
> +  }
> +  /// getDirectNonVBaseClassToDestroy - Get the virtual base.
> +  Type *getDirectNonVBaseClassToDestroy(uintptr_t Base) const {
> +    if (isDirectNonVBaseToDestroy(Base))
> +      return reinterpret_cast<Type*>(Base  & ~0x02);
> +    return 0;
> +  }

These hardcoded 0x01, 0x02, and 0x03 constants can be tricky to  
follow. Could you create an enum that gives useful names to these  
values?

> +  /// computeBaseOrMembersToDestroy - Compute information in current
> +  /// destructor decl's AST of bases and non-static data members  
> which will be
> +  /// implicitly destroyed. We are storing the destruction in the  
> order that
> +  /// they should occur (which is the reverse of construction order).
> +  void computeBaseOrMembersToDestroy(ASTContext &C);

Thanks!

> void
> -CXXDestructorDecl::setBaseOrMemberDestructions(ASTContext &C) {
> +CXXDestructorDecl::Destroy(ASTContext& C) {
> +  C.Deallocate(BaseOrMemberDestructions);
> +  CXXMethodDecl::Destroy(C);
> +}

Looks good!

> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=76663&r1=76662&r2=76663&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Jul 21 17:36:06 2009
> @@ -3282,12 +3282,15 @@
>   // Verify that that gotos and switch cases don't jump into scopes  
> illegally.
>   if (CurFunctionNeedsScopeChecking)
>     DiagnoseInvalidJumps(Body);
> -
> -  // C++ constructors that have function-try-blocks can't have  
> return statements
> -  // in the handlers of that block. (C++ [except.handle]p14) Verify  
> this.
> +
> +  // C++ constructors that have function-try-blocks can't have return
> +  // statements in the handlers of that block. (C++ [except.handle] 
> p14)
> +  // Verify this.
>   if (isa<CXXConstructorDecl>(dcl) && isa<CXXTryStmt>(Body))
>     DiagnoseReturnInConstructorExceptionHandler(cast<CXXTryStmt> 
> (Body));
> -
> +
> +  if (CXXDestructorDecl *Destructor = dyn_cast<CXXDestructorDecl> 
> (dcl))
> +    Destructor->computeBaseOrMembersToDestroy(Context);
>   return D;
> }

I really like this. Thanks!

   - Doug