[cfe-commits] r76776 - /cfe/trunk/lib/AST/DeclCXX.cpp

[Daniel Dunbar]

On Wed, Jul 22, 2009 at 1:25 PM, Fariborz Jahanian<fjahanian at apple.com> wrote:
> Author: fjahanian
> Date: Wed Jul 22 15:25:00 2009
> New Revision: 76776
>
> URL: http://llvm.org/viewvc/llvm-project?rev=76776&view=rev
> Log:
> Improved on performance of the algorithm for proper ordering of
> ctor's initialization of bases and fields.

Cool

> +  llvm::DenseMap<const void *, CXXBaseOrMemberInitializer*> AllBaseFields;
> +
> +  for (unsigned i = 0; i < NumInitializers; i++) {
> +    CXXBaseOrMemberInitializer *Member = Initializers[i];
> +    const void * Key = Member->isBaseInitializer() ?
> +      reinterpret_cast<const void *>(
> +                                   Member->getBaseClass()->getAsRecordType()) :
> +      reinterpret_cast<const void *>(Member->getMember());
> +    AllBaseFields[Key] = Member;
> +  }

I think this is more readable as:
--
for ... {
    CXXBaseOrMemberInitializer *Member = Initializers[i];
    if (Member->isBaseInitializer())
      AllBaseFields[Member->getBaseClass()->getAsRecordType()] = Member;
    else
      AllBaseFields[Member->getMember()] = Member;
}
--

> +
>   // Push virtual bases before others.
>   for (CXXRecordDecl::base_class_iterator VBase =
>        ClassDecl->vbases_begin(),
>        E = ClassDecl->vbases_end(); VBase != E; ++VBase) {
> -    const Type * T = VBase->getType()->getAsRecordType();
> -    unsigned int i = 0;
> -    for (i = 0; i < NumInitializers; i++) {
> -      CXXBaseOrMemberInitializer *Member = Initializers[i];
> -      if (Member->isBaseInitializer() &&
> -          Member->getBaseClass()->getAsRecordType() == T) {
> -        AllToInit.push_back(Member);
> -        break;
> -      }
> -    }
> -    if (i == NumInitializers) {
> +    const void *Key = reinterpret_cast<const void *>(
> +                                          VBase->getType()->getAsRecordType());

I don't think this case is necessary?

> +    if (AllBaseFields[Key])
> +      AllToInit.push_back(AllBaseFields[Key]);

This should be
--
if (CXXBaseOrMemberInitializer *Value = AllBaseFields.lookup(Key))
  AllToInit.push_back(Value)
--
Otherwise it looks up the value twice, and inserts a null binding into the map.

> +    const void *Key = reinterpret_cast<const void *>(
> +                                          Base->getType()->getAsRecordType());
> +    if (AllBaseFields[Key])
> +      AllToInit.push_back(AllBaseFields[Key]);

Same two comments as before.

> +    const void * Key = reinterpret_cast<const void *>(*Field);
> +    if (AllBaseFields[Key]) {
> +      AllToInit.push_back(AllBaseFields[Key]);
> +      continue;

Ditto.

 - Daniel