Re: [PATCH] gdb bug 12417

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Hello Mohsan,

Thanks for the patch.  Not really a technical review, but rather
formatting nits.

On Monday, September 24 2012, Mohsan Saleem wrote:

> diff -ruN ./gdb_old/ChangeLog ./gdb_new/ChangeLog
> --- ./gdb_old/ChangeLog	2012-09-24 13:17:34.000000000 +0500
> +++ ./gdb_new/ChangeLog	2012-09-24 22:25:44.465441936 +0500
> @@ -1,3 +1,12 @@
> +2012-09-24  Mohsan Saleem  <mohsansaleem_ms@yahoo.com>
> +
> +	* thread.c (thread_name): New function. To get thread name
                                               ^^

Two spaces after period.  Missing period at the end of the sentence.
You also don't need to explain what `thread_name' does, therefore you
can get rid of the "To get thread name" part.

> +	(add_thread_with_info): Update to print thread name.
> +	(thread_apply_all_command): Likewise.
> +	(thread_apply_command): Likewise.
> +	(thread_find_command): Likewise.
> +	(do_captured_thread_select): Likewise.

Since you are solving a PR, it is recommended that you put its reference
on the ChangeLog message, like this:

2012-09-24  Mohsan Saleem  <mohsansaleem_ms@yahoo.com>

	PR threads/12417
	...
	
> diff -ruN ./gdb_old/gdb/testsuite/gdb.threads/thread-find.exp ./gdb_new/gdb/testsuite/gdb.threads/thread-find.exp
> --- ./gdb_old/gdb/testsuite/gdb.threads/thread-find.exp	2012-09-24 13:17:38.000000000 +0500
> +++ ./gdb_new/gdb/testsuite/gdb.threads/thread-find.exp	2012-09-24 21:55:44.737537299 +0500
> @@ -186,17 +186,17 @@
>  
>  if { [info exists thread6] } then {
>      gdb_test "thread find $thread6" \
> -	"Thread 6 has .*$thread6.*" "find thread id 6"
> +	"Thread 6 threadname_6 has .*$thread6.*" "find thread id 6"

I have some comments about the printing style, see below.

> diff -ruN ./gdb_old/gdb/thread.c ./gdb_new/gdb/thread.c
> --- ./gdb_old/gdb/thread.c	2012-09-24 13:17:37.000000000 +0500
> +++ ./gdb_new/gdb/thread.c	2012-09-24 21:54:46.689540365 +0500
> @@ -64,6 +64,7 @@
>  static void thread_apply_command (char *, int);
>  static void restore_current_thread (ptid_t);
>  static void prune_threads (void);
> +static const char* thread_name (struct thread_info *);
                    ^

The `*' must be together with `thread_name', not `char':

    static const char *thread_name (struct thread_info *);

> +const char*
             ^

Should be:

     const char *

> +thread_name (struct thread_info *ti)
> +{
> +  const char* name;
               ^

Should be:

     const char *name;

> +  name = ti->name ? ti->name : target_thread_name(ti);

The standard is to explict check for NULL.  Also, there must be one
space between the function name and the open paren.

     name = ti->name != NULL ? ti-> name : target_thread_name (ti);

> +  if (name)
> +    return name;
> +  else
> +    return "";

You could do:

    return name != NULL ? name : "";

This whole function could also be shortened, but I think it's OK as-is.

>  void
>  delete_step_resume_breakpoint (struct thread_info *tp)
>  {
> @@ -236,7 +248,7 @@
>    result->private = private;
>  
>    if (print_thread_events)
> -    printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
> +    printf_unfiltered (_("[New %s %s]\n"), target_pid_to_str (ptid), thread_name (result));
>  
>    annotate_new_thread ();
>    return result;
> @@ -1198,8 +1210,8 @@
>        {
>  	switch_to_thread (tp->ptid);
>  
> -	printf_filtered (_("\nThread %d (%s):\n"),
> -			 tp->num, target_pid_to_str (inferior_ptid));
> +	printf_filtered (_("\nThread %d %s (%s):\n"), tp->num,
> +			 thread_name (tp), target_pid_to_str (inferior_ptid));

I would like to hear others' opinions, but I guess this line could be
printed differently.  Currently it is:

        Thread 2 thread_name2 (12323)...

It could now be:

        Thread thread_name2 [#2] (12323)...

I think it gives more importance to the name.  The same comments applies
to all other commands.  But please, don't change anything yet, wait
until a maintainer states his opinions.

WDYT?

-- 
Sergio