[llvm-commits] [llvm] r147651 - in /llvm/trunk/lib/CodeGen/AsmPrinter: DwarfAccelTable.cpp DwarfAccelTable.h DwarfCompileUnit.cpp DwarfCompileUnit.h DwarfDebug.cpp

[Devang Patel]

Eric,

Couple of nit picks and one performance concern....

On Jan 5, 2012, at 8:35 PM, Eric Christopher wrote:

> @@ -608,8 +609,20 @@
>   }
>   // If this is a named finished type then include it in the list of types
>   // for the accelerator tables.
> -  if (!Ty.getName().empty() && !Ty.isForwardDecl())
> -    addAccelType(Ty.getName(), TyDIE);
> +  if (!Ty.getName().empty() && !Ty.isForwardDecl()) {
> +    bool IsImplementation = 0;
> +    if (Ty.isCompositeType()) {
> +      DICompositeType CT(Ty);
> +      IsImplementation = (CT.getRunTimeLang() == 0) ||
> +        CT.isObjcClassComplete();;
> +    }

I'd push this in DebugInfo and simply use DICompositeType.isImplementation() here.
Please add comment clarifying how you're treating C++ here (:)) and why.

> +    
> +    addAccelType(Ty.getName(),
> +                 std::make_pair(TyDIE,
> +                                (IsImplementation ?
> +                               DwarfAccelTable::eTypeFlagClassIsImplementation :
> +                                 0)));
> +  }

For sake of readers eye, I'd just hoist ? and std::make_pair outside the call and trust optimizer.

> 
>   addToContextOwner(TyDIE, Ty.getContext());
>   return TyDIE;
> 
> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h?rev=147651&r1=147650&r2=147651&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h (original)
> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h Thu Jan  5 22:35:23 2012
> @@ -65,7 +65,7 @@
>   StringMap<std::vector<DIE*> > AccelNames;
>   StringMap<std::vector<DIE*> > AccelObjC;
>   StringMap<std::vector<DIE*> > AccelNamespace;
> -  StringMap<std::vector<DIE*> > AccelTypes;
> +  StringMap<std::vector<std::pair<DIE*, unsigned> > > AccelTypes;

It is a good idea to have typedef here. I see vector<pair<> > multiple times.

> 
> -      std::vector<DIE *> Entities = GI->second;
> -      for (std::vector<DIE *>::const_iterator DI = Entities.begin(),
> -             DE= Entities.end(); DI !=DE; ++DI)
> -        AT.AddName(Name, (*DI));
> +      std::vector<std::pair<DIE *, unsigned> > Entities = GI->second;

Do you really want to copy the entire vector or just get a reference to iterate over ?

> +      for (std::vector<std::pair<DIE *, unsigned> >::const_iterator DI
> +             = Entities.begin(), DE = Entities.end(); DI !=DE; ++DI)
> +        AT.AddName(Name, (*DI).first, (*DI).second);
>     }
>   }

-
Devang