From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1643 invoked by alias); 26 Jun 2011 20:49:02 -0000 Received: (qmail 1621 invoked by uid 22791); 26 Jun 2011 20:49:01 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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; Sun, 26 Jun 2011 20:48:41 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p5QKmeq0013580 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 26 Jun 2011 16:48:40 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p5QKmcIJ016673; Sun, 26 Jun 2011 16:48:39 -0400 From: Phil Muldoon To: iam ahal Cc: gdb-patches@sourceware.org Subject: Re: [patch] GDB 7.2: new feature for "backtrace" that cuts path to file (remain filename) References: Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Sun, 26 Jun 2011 20:49:00 -0000 In-Reply-To: (iam ahal's message of "Mon, 27 Jun 2011 00:00:28 +0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) 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: 2011-06/txt/msg00386.txt.bz2 iam ahal writes: You could use Python to write a custom backtrace to do this. But that is neither here or there to this patch. I have no opinion on the patch implementation itself, but a few nits. > ChangeLog: > > 2011-06-26 Eldar Gaynetdinov : No ":" at the end of that line. > * stack.c (backtrace_command): Created new variable "nofull_path" for > implementation of "backtrace nopath" command. > It has similar logic as "fulltrace_arg" > for "backtrace full" command. > (backtrace_full_command): nofull_path is just zero (i.e. > it's not used there). > (backtrace_command_stub): just pass new argument > ("args->nofull_path") to backtrace_command_1 > (backtrace_command_1): if "nofull_path" is enabled (by > "backtrace nopath") then call print_frame_info with LOC_NO_FULLPATH. > (print_frame_info): work with LOC_NO_FULLPATH same as > LOCATION (because it's almost same thing). > (print_frame): if LOC_NO_FULLPATH was passed then cut > fullpath (if exist) and remain only filename. > * frame.h (enum print_what): Added LOC_NO_FULLPATH with comment. A few small things. The general rule of ChangeLog files is "what" changed, not "why". Proper punctuation and capitalisation also. The indenting seems off, too. Any continuation of a comment should continue at the first "(" of the previous function in a ChangeLog. > diff -rup -x configure gdb-7.2-orig/gdb/frame.h gdb-7.2/gdb/frame.h > --- gdb-7.2-orig/gdb/frame.h 2010-01-01 10:31:32.000000000 +0300 > +++ gdb-7.2/gdb/frame.h 2011-06-26 21:56:52.000000000 +0400 > @@ -582,7 +582,10 @@ enum print_what > /* Print both of the above. */ > SRC_AND_LOC, > /* Print location only, but always include the address. */ > - LOC_AND_ADDRESS > + LOC_AND_ADDRESS, > + /* Print only the location but without the full path to file, * > + * i.e. print only filename even if full path is defined in symtable. */ > + LOC_NO_FULLPATH > }; Two spaces after "." even before the */. > location_print = (print_what == LOCATION > || print_what == LOC_AND_ADDRESS > - || print_what == SRC_AND_LOC); > + || print_what == SRC_AND_LOC > + || print_what == LOC_NO_FULLPATH); Not sure if this is an issue with your editor, mail, or just plain patch weirdness, but this indention looks off. The first + line is indented correctly, the second looks like it is missing a tab. > - if (print_what != LOCATION) > + if (print_what != LOCATION || print_what != LOC_NO_FULLPATH) > set_default_breakpoint (1, sal.pspace, > get_frame_pc (frame), sal.symtab, sal.line); Indention, might be the patch interpretation too. > - ui_out_field_string (uiout, "file", sal.symtab->filename); > + > + filename = NULL; > + if (print_what == LOC_NO_FULLPATH) > + { > + filename = strrchr( sal.symtab->filename, '/' ); > + if (filename != NULL) > + filename++; > + } Indention, and space between function name and (. IE, strrchr (. > /* Stub for catch_errors. */ > @@ -1384,7 +1403,7 @@ backtrace_command_stub (void *data) > { > struct backtrace_command_args *args = data; > > - backtrace_command_1 (args->count_exp, args->show_locals, args->from_tty); > + backtrace_command_1 (args->count_exp, args->show_locals, args->from_tty, args->nofull_path); This function probably need to be wrapped onto the next line. > if (fulltrace_arg < 0 && subset_compare (argv[i], "full")) > fulltrace_arg = argc; > + else if (nofull_path < 0 && subset_compare (argv[i], "nopath")) > + nofull_path = argc; > else > { Indention looks a little off. The "else" should match the preceding "if" and "nofull_patch..." line match the indention of the "fulltrace_argc = arg" line. Once again this may be a mailer issue on my part, but some of your indention looks correct, while others look off. Anyway please check. You should have a testing statement indicating that it has not regressed any existing functionality, as well as the architecture/distro you tested it on. So you should run the GDB testsuite to test the effects of your patch, before and after. Also as this patch introduces new functionality, you should write tests that test that functionality. This helps prove the patch, and help the maintainers catch future regressions. This patch probably need a documentation patch to document the new backtrace option. Finally, I am not a maintainer, so don't take anything I have said as approval for the patch. ;) One of the maintainers will separately comment on it. Thanks for your effort, and your patch! Cheers, Phil