Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Joel Brobecker <brobecker@adacore.com>
To: Keven Boell <keven.boell@linux.intel.com>
Cc: Keven Boell <keven.boell@intel.com>, gdb-patches@sourceware.org
Subject: Re: [PATCH] Introduce linked list for dynamic attributes
Date: Mon, 16 Mar 2015 19:55:00 -0000	[thread overview]
Message-ID: <20150316195500.GA3435@adacore.com> (raw)
In-Reply-To: <54FEDD80.1020102@linux.intel.com>

> From: Keven Boell <keven.boell@intel.com>
> Date: Mon, 23 Feb 2015 15:45:06 +0100
> Subject: [PATCH] Introduce linked list for dynamic attributes
> 
> This patch introduces a linked list for dynamic
> attributes of a type. This is a pre-work for
> the Fortran dynamic array support. The Fortran
> dynamic array support will add more dynamic
> attributes to a type. As only a few types
> will have such dynamic attributes set, a linked
> list is more efficient in terms of memory
> consumption than adding multiple attributes
> to main_type.
> 
> Transformed the data_location dynamic attribute
> to use the linked list.
> 
> 2015-02-23  Keven Boell  <keven.boell@intel.com>
> 
> 	* gdbtypes.c (resolve_dynamic_type_internal):
> 	Adapt data_location usage to linked list.
> 	(resolve_dynamic_type_internal): Adapt
> 	data_location usage to linked list.
> 	(get_dyn_prop): New function.
> 	(add_dyn_prop): New function.
> 	(copy_dynamic_prop_list): New function.
> 	(copy_type_recursive): Add copy of linked
> 	list.
> 	(copy_type): Add copy of linked list.
> 	* gdbtypes.h (enum dynamic_prop_node_kind):
> 	Kind of the dynamic attribute in linked list.
> 	(struct dynamic_prop_list): Dynamic list
> 	node.
> 	* dwarf2read.c (set_die_type): Add data_location
> 	data to linked list using helper functions.

OK to push, but before  you do, make sure you fix up the date
in the ChangeLog entry both in the revision history (the one quoted
above) and the one in the ChangeLog file.

Also, would you mind reformatting the revision log to something that
uses a slightly longer line length? Something like 70 characters will
allow us to use fewer lines, and make the revision log a little
shorter (as well as more readable, IMO, but that may be just me).

And while looking at the contents of the ChangeLog one more time,
I think I missed on some suggestions. So below is what I suggest.
Please take a look to see what the changes are and whether you
agree with them. Basically, the ChangeLog says what you did in
concise terms, now what the different new elements are for.

| This patch introduces a linked list for dynamic attributes of a type.
| This is a pre-work for the Fortran dynamic array support. The Fortran
| dynamic array support will add more dynamic attributes to a type.
| As only a few types will have such dynamic attributes set, a linked
| list is more efficient in terms of memory consumption than adding
| multiple attributes to main_type.
| 
| 2015-03-16  Keven Boell  <keven.boell@intel.com>
| 
| 	* gdbtypes.c (resolve_dynamic_type_internal): Adapt
| 	data_location usage to linked list.
| 	(resolve_dynamic_type_internal): Adapt data_location to
| 	linked list.
| 	(get_dyn_prop, add_dyn_prop, copy_dynamic_prop_list): New function.
| 	(copy_type_recursive, copy_type): Add copy of linked list.
| 	* gdbtypes.h (enum dynamic_prop_node_kind): New enum.
| 	(struct dynamic_prop_list): New struct.
| 	* dwarf2read.c (set_die_type): Set data_location data.

Please make sure that this ChangeLog entry matches what you put
in gdb/ChangeLog.

Other than that, well done, and thank you for the patch. We can now
move to the next phase of your patch series!

-- 
Joel


  reply	other threads:[~2015-03-16 19:55 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-02  7:37 Keven Boell
2015-03-02 19:50 ` Joel Brobecker
2015-03-03 13:43   ` Keven Boell
2015-03-03 15:55     ` Joel Brobecker
2015-03-04 12:27       ` Keven Boell
2015-03-05 18:13         ` Joel Brobecker
2015-03-10 12:03           ` Keven Boell
2015-03-16 19:55             ` Joel Brobecker [this message]
2015-03-17 11:43               ` Keven Boell
2015-03-20 21:54                 ` Joel Brobecker
2015-03-03 13:43   ` Keven Boell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150316195500.GA3435@adacore.com \
    --to=brobecker@adacore.com \
    --cc=gdb-patches@sourceware.org \
    --cc=keven.boell@intel.com \
    --cc=keven.boell@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox