[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