[RFA 2/2] Use std::vector for field lists in dwarf2read.c

Simon Marchi simark@simark.ca
Sun Mar 11 17:32:00 GMT 2018


On 2018-03-11 12:19 AM, Tom Tromey wrote:
> This changes dwarf2read.c to use std::vector rather than a linked list
> when managing the fields and base classes to be added to a type.  This
> removes some bookkeeping types and also allows the removal of some
> cleanups.

That was a good case of twisted and obfuscated code.  For example:

  for (k = flp->length; (k--, nfp); nfp = nfp->next)
    fn_flp->fn_fields[k] = nfp->fnfield;

Your patch makes the code more boring and readable, which is good :).

I didn't spot anything wrong, just some nits.  The patch LGTM with
those fixed.

> @@ -15475,43 +15454,35 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
>  {
>    struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
>    struct gdbarch *gdbarch = get_objfile_arch (objfile);
> -  struct nextfield *new_field;
>    struct attribute *attr;
>    struct field *fp;
>    const char *fieldname = "";
>  
> -  /* Allocate a new field list entry and link it in.  */
> -  new_field = XNEW (struct nextfield);
> -  make_cleanup (xfree, new_field);
> -  memset (new_field, 0, sizeof (struct nextfield));
> -
>    if (die->tag == DW_TAG_inheritance)
> -    {
> -      new_field->next = fip->baseclasses;
> -      fip->baseclasses = new_field;
> -    }
> +    fip->baseclasses.emplace_back ();
>    else
> -    {
> -      new_field->next = fip->fields;
> -      fip->fields = new_field;
> -    }
> +    fip->fields.emplace_back ();
> +  struct nextfield &new_field
> +    = ((die->tag == DW_TAG_inheritance)
> +       ? fip->baseclasses : fip->fields).back ();

I think this construct is not very readable.  I would prefer keeping new_field as
a pointer, and setting it in each branch of the previous if.

> @@ -15926,49 +15865,33 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
>      return;
>  
>    /* Look up member function name in fieldlist.  */
> -  for (i = 0; i < fip->nfnfields; i++)
> +  for (i = 0; i < fip->fnfieldlists.size (); i++)
>      {
>        if (strcmp (fip->fnfieldlists[i].name, fieldname) == 0)
> -	break;
> +	{
> +	  flp = &fip->fnfieldlists[i];
> +	  break;
> +	}
>      }
>  
>    /* Create new list element if necessary.  */

This comment is stale.

> -  if (i < fip->nfnfields)
> -    flp = &fip->fnfieldlists[i];
> -  else
> +  if (flp == nullptr)
>      {
> -      if ((fip->nfnfields % DW_FIELD_ALLOC_CHUNK) == 0)
> -	{
> -	  fip->fnfieldlists = (struct fnfieldlist *)
> -	    xrealloc (fip->fnfieldlists,
> -		      (fip->nfnfields + DW_FIELD_ALLOC_CHUNK)
> -		      * sizeof (struct fnfieldlist));
> -	  if (fip->nfnfields == 0)
> -	    make_cleanup (free_current_contents, &fip->fnfieldlists);
> -	}
> -      flp = &fip->fnfieldlists[fip->nfnfields];
> +      fip->fnfieldlists.emplace_back ();
> +      flp = &fip->fnfieldlists.back ();
>        flp->name = fieldname;
> -      flp->length = 0;
> -      flp->head = NULL;
> -      i = fip->nfnfields++;
> +      i = fip->fnfieldlists.size () - 1;
>      }
>  
>    /* Create a new member function field and chain it to the field list
>       entry.  */

This one too.

> -  new_fnfield = XNEW (struct nextfnfield);
> -  make_cleanup (xfree, new_fnfield);
> -  memset (new_fnfield, 0, sizeof (struct nextfnfield));
> -  new_fnfield->next = flp->head;
> -  flp->head = new_fnfield;
> -  flp->length++;
> -
> -  /* Fill in the member function field info.  */
> -  fnp = &new_fnfield->fnfield;
> +  flp->fnfields.emplace_back ();
> +  fnp = &flp->fnfields.back ();
>  
>    /* Delay processing of the physname until later.  */
>    if (cu->language == language_cplus)
>      {
> -      add_to_method_list (type, i, flp->length - 1, fieldname,
> +      add_to_method_list (type, i, flp->fnfields.size () - 1, fieldname,
>  			  die, cu);
>      }

Nit: If you remove the unnecessary braces here, you can avoid the line wrapping.

>    else
> @@ -16117,7 +16040,6 @@ static void
>  dwarf2_attach_fn_fields_to_type (struct field_info *fip, struct type *type,
>  				 struct dwarf2_cu *cu)
>  {
> -  struct fnfieldlist *flp;
>    int i;
>  
>    if (cu->language == language_ada)
> @@ -16125,23 +16047,25 @@ dwarf2_attach_fn_fields_to_type (struct field_info *fip, struct type *type,
>  
>    ALLOCATE_CPLUS_STRUCT_TYPE (type);
>    TYPE_FN_FIELDLISTS (type) = (struct fn_fieldlist *)
> -    TYPE_ALLOC (type, sizeof (struct fn_fieldlist) * fip->nfnfields);
> +    TYPE_ALLOC (type,
> +		sizeof (struct fn_fieldlist) * fip->fnfieldlists.size ());
>  
> -  for (i = 0, flp = fip->fnfieldlists; i < fip->nfnfields; i++, flp++)
> +  for (i = 0; i < fip->fnfieldlists.size (); i++)

Variables i and k can be declared in the for loops.

>      {
> -      struct nextfnfield *nfp = flp->head;
> +      struct fnfieldlist &nf = fip->fnfieldlists[i];
>        struct fn_fieldlist *fn_flp = &TYPE_FN_FIELDLIST (type, i);
>        int k;
>  
> -      TYPE_FN_FIELDLIST_NAME (type, i) = flp->name;
> -      TYPE_FN_FIELDLIST_LENGTH (type, i) = flp->length;
> +      TYPE_FN_FIELDLIST_NAME (type, i) = nf.name;
> +      TYPE_FN_FIELDLIST_LENGTH (type, i) = nf.fnfields.size ();
>        fn_flp->fn_fields = (struct fn_field *)
> -	TYPE_ALLOC (type, sizeof (struct fn_field) * flp->length);
> -      for (k = flp->length; (k--, nfp); nfp = nfp->next)
> -	fn_flp->fn_fields[k] = nfp->fnfield;
> +	TYPE_ALLOC (type, sizeof (struct fn_field) * nf.fnfields.size ());
> +
> +      for (k = 0; k < nf.fnfields.size (); ++k)
> +	fn_flp->fn_fields[k] = nf.fnfields[k];
>      }
>  
> -  TYPE_NFN_FIELDS (type) = fip->nfnfields;
> +  TYPE_NFN_FIELDS (type) = fip->fnfieldlists.size ();
>  }
>  
>  /* Returns non-zero if NAME is the name of a vtable member in CU's
> @@ -16419,11 +16343,11 @@ handle_struct_member_die (struct die_info *child_die, struct type *type,
>  
>        /* The first field was just added, so we can stash the
>  	 discriminant there.  */
> -      gdb_assert (fi->fields != NULL);
> +      gdb_assert (!fi->fields.empty ());
>        if (discr == NULL)
> -	fi->fields->variant.default_branch = true;
> +	fi->fields.back ().variant.default_branch = true;
>        else
> -	fi->fields->variant.discriminant_value = DW_UNSND (discr);
> +	fi->fields.back ().variant.discriminant_value = DW_UNSND (discr);
>      }
>  }
>  
> @@ -16478,9 +16402,6 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
>      {
>        struct field_info fi;
>        std::vector<struct symbol *> template_args;
> -      struct cleanup *back_to = make_cleanup (null_cleanup, 0);
> -
> -      memset (&fi, 0, sizeof (struct field_info));
>  
>        child_die = die->child;
>  
> @@ -16489,7 +16410,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
>  	  handle_struct_member_die (child_die, type, &fi, &template_args, cu);
>  
>  	  if (is_variant_part && discr_offset == child_die->sect_off)
> -	    fi.fields->variant.is_discriminant = true;
> +	    fi.fields.back ().variant.is_discriminant = true;
>  
>  	  child_die = sibling_die (child_die);
>  	}
> @@ -16512,7 +16433,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
>        /* Attach fields and member functions to the type.  */
>        if (fi.nfields)
>  	dwarf2_attach_fields_to_type (&fi, type, cu);
> -      if (fi.nfnfields)
> +      if (!fi.fnfieldlists.empty ())
>  	{
>  	  dwarf2_attach_fn_fields_to_type (&fi, type, cu);
>  
> @@ -16582,9 +16503,9 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
>  
>        /* Copy fi.typedef_field_list linked list elements content into the
>  	 allocated array TYPE_TYPEDEF_FIELD_ARRAY (type).  */
> -      if (fi.typedef_field_list)
> +      if (!fi.typedef_field_list.empty ())
>  	{
> -	  int i = fi.typedef_field_list_count;
> +	  int i = fi.typedef_field_list.size ();

Can you rename this variable to something else (size, count, etc) and declare
the int i in the for loop?  The two usages of i are not related anymore, so it
would be clearer with two separate variables.

>  
>  	  ALLOCATE_CPLUS_STRUCT_TYPE (type);
>  	  TYPE_TYPEDEF_FIELD_ARRAY (type)
> @@ -16592,23 +16513,15 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
>  	       TYPE_ALLOC (type, sizeof (TYPE_TYPEDEF_FIELD (type, 0)) * i));
>  	  TYPE_TYPEDEF_FIELD_COUNT (type) = i;
>  
> -	  /* Reverse the list order to keep the debug info elements order.  */
> -	  while (--i >= 0)
> -	    {
> -	      struct decl_field *dest, *src;
> -
> -	      dest = &TYPE_TYPEDEF_FIELD (type, i);
> -	      src = &fi.typedef_field_list->field;
> -	      fi.typedef_field_list = fi.typedef_field_list->next;
> -	      *dest = *src;
> -	    }
> +	  for (i = 0; i < fi.typedef_field_list.size (); ++i)
> +	    TYPE_TYPEDEF_FIELD (type, i) = fi.typedef_field_list[i];
>  	}
>  
>        /* Copy fi.nested_types_list linked list elements content into the
>  	 allocated array TYPE_NESTED_TYPES_ARRAY (type).  */
> -      if (fi.nested_types_list != NULL && cu->language != language_ada)
> +      if (!fi.nested_types_list.empty () && cu->language != language_ada)
>  	{
> -	  int i = fi.nested_types_list_count;
> +	  int i = fi.nested_types_list.size ();

Same here.

Thanks!

Simon



More information about the Gdb-patches mailing list