From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29149 invoked by alias); 24 Sep 2012 22:19:16 -0000 Received: (qmail 29115 invoked by uid 22791); 24 Sep 2012 22:19:15 -0000 X-SWARE-Spam-Status: No, hits=-7.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 24 Sep 2012 22:18:57 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q8OMIuGI026635 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 24 Sep 2012 18:18:56 -0400 Received: from psique (ovpn-113-44.phx2.redhat.com [10.3.113.44]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q8OMIsoe025308; Mon, 24 Sep 2012 18:18:55 -0400 From: Sergio Durigan Junior To: Mohsan Saleem Cc: "gdb-patches\@sourceware.org" Subject: Re: [PATCH] gdb bug 12417 References: <1348508232.46785.YahooMailNeo@web114411.mail.gq1.yahoo.com> X-URL: http://www.redhat.com Date: Mon, 24 Sep 2012 22:19:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-09/txt/msg00529.txt.bz2 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 > + > + * 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 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