Re: [PATCH]Add symbol whose field 'has_type' has been set to partial symbol table

[Yao Qi <yao at codesourcery dot com>]

On 01/24/2014 02:19 PM, Jun Gong wrote:
> This patch intends to add symbol whose field 'has_type' has been set
> to partial symbol table.
> 
> The patch and corresponding test case have been discussed and reviewed
> before at https://sourceware.org/ml/gdb-patches/2013-10/msg00747.html
> and https://sourceware.org/ml/gdb-patches/2013-11/msg00003.html.
> However it has not been committed because Tom Tromey is very busy and
> could not help commit it now. I have received copyright assignment.
> Could you help review and commit the patch? Thank you!

Thanks for submitting this patch.  Patch itself looks simple, but
doesn't meet some requirements for patch submission.  In short,
please have a look at https://sourceware.org/gdb/wiki/ContributionChecklist

> 
> The following are the summary of the patch, and the attachment
> const-var.exp is test case.
> 
> 1. Change for gdb/ChangeLog
> 
> 2014-01-24  Jun Gong  <heixia108@gmail.com>
> 
>           * dwarf2read.c(add_partial_symbol): Add symbol whose field

Each entry should be tab-prefixed.  A space is missing before "(".

> 'has_type' has been set to partial symbol table.

Should be tab-prefixed.

> 
> 2. Diff for dwarf2read.c
> 
> diff -up gdb-7.7.50.20140124.orig/gdb/dwarf2read.c
> gdb-7.7.50.20140124/gdb/dwarf2read.c
> --- gdb-7.7.50.20140124.orig/gdb/dwarf2read.c 2014-01-24
> 13:04:04.169111566 +0800
> +++ gdb-7.7.50.20140124/gdb/dwarf2read.c 2014-01-24 13:26:48.445122774 +0800
> @@ -6769,8 +6769,9 @@ add_partial_symbol (struct partial_die_i
>   }
>        else
>   {
> -  /* Static Variable.  Skip symbols without location descriptors.  */
> -  if (pdi->d.locdesc == NULL)
> +  /* Static Variable.  Skip symbols without location descriptors. or
> +      has_type not set. */

The period after "descriptors" should be removed.  It is better to say

"Skip symbols without location descriptors or don't have type", or
whatever "pdi->has_type == 0" stands for.

> +  if (pdi->d.locdesc == NULL && pdi->has_type == 0)

pdi->has_type is a boolean to me, we can use "!pdi->has_type".

>      {
>        xfree (built_actual_name);
>        return;
> 
> 3. Change for gdb/testsuite/Changelog
> 
> 2014-01-24  Jun Gong  <heixia108@gmail.com>
> 
>       * gdb.dwarf2/const-var.exp: New file.
> 
> 4. Test case has been attached as const-var.exp.
> 

Attaching a new file gets the review a little hard.  Please generate a
diff including your changes above and this new file.  That is easier
for people to review.  Write the changelog in the e-mail together,

gdb:

2014-01-24  Jun Gong  <heixia108@gmail.com>

	* dwarf2read.c(add_partial_symbol): Add symbol whose field
	'has_type' has been set to partial symbol table.

gdb/testsuite:

2014-01-24  Jun Gong  <heixia108@gmail.com>

      * gdb.dwarf2/const-var.exp: New file.

and attach the patch including the changes in dwarf2read.c and new test
cases in the e-mail.

-- 
Yao (éå)