From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14616 invoked by alias); 2 Nov 2011 22:53:44 -0000 Received: (qmail 14604 invoked by uid 22791); 2 Nov 2011 22:53:42 -0000 X-SWARE-Spam-Status: No, hits=-3.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-qw0-f41.google.com (HELO mail-qw0-f41.google.com) (209.85.216.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 02 Nov 2011 22:53:27 +0000 Received: by qadc11 with SMTP id c11so808955qad.0 for ; Wed, 02 Nov 2011 15:53:27 -0700 (PDT) Received: by 10.224.215.73 with SMTP id hd9mr3131667qab.94.1320274405745; Wed, 02 Nov 2011 15:53:25 -0700 (PDT) MIME-Version: 1.0 Received: by 10.224.215.73 with SMTP id hd9mr3131632qab.94.1320274404526; Wed, 02 Nov 2011 15:53:24 -0700 (PDT) Received: by 10.224.6.76 with HTTP; Wed, 2 Nov 2011 15:53:24 -0700 (PDT) In-Reply-To: References: <20110627160029.GF20676@adacore.com> <834o33qlm9.fsf@gnu.org> <83bowq6x7f.fsf@gnu.org> Date: Wed, 02 Nov 2011 22:53:00 -0000 Message-ID: Subject: Re: [patch] GDB 7.2: new feature for "backtrace" that cuts path to file (remain filename) From: Doug Evans To: iam ahal Cc: Tom Tromey , gdb-patches@sourceware.org, eliz@gnu.org, pmuldoon@redhat.com, brobecker@adacore.com, pedro@codesourcery.com, drow@false.org, jan.kratochvil@redhat.com Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true 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-11/txt/msg00058.txt.bz2 On Sun, Oct 30, 2011 at 11:16 AM, iam ahal wrote: > Copyright assignment by [gnu.org #703515] is completed. > I've corrected some pieces of the patch by your notes. > Also, I've attached the change log. > > Please, check it. > > Thank you. > > ~Eldar. Hi. Here's my $0.02 review. These are all nits. --- For your changelog entry: [these are all for consistency with existing entries] Add a new variable that controls a way in which filenames in backtraces is displayed. For this entry: * gdb.texinfo: Added description of 'filename-display' variable in 'set/show backtrace' section. Specify which @node the gdb.texinfo entry is for. Existing example: =A0 =A0 =A0 =A0* gdb.texinfo (Values From Inferior): Add is_lazy attribute, =A0 =A0 =A0 =A0fetch_lazy method. For this entry: * frame.c: Added including of a header file. =A0Added definition of three global string variables and global array of string variables. Added definition of one global string variable. Specify what the variable names are. Existing example: =A0 =A0 =A0 =A0* infrun.c (disable_randomization): New global variable. (show_filename_display_string): New function. For this entry: (get_filename_display_from_sal): New function with commentary. No need to write "with commentary" here or below. (_initialize_frame): Added initialization of 'filename-display' variable. For this entry: * frame.h: Added declaration of a new function with commentary. You need to write the name of the function here. Existing example: =A0 =A0 =A0 =A0* value.h (read_frame_register_value): Add declaration. * stack.c (print_frame): Added new variable and calling of a new function and condition with this variable. Changed third argument of calling of a function. --- For your patch: I wish I could think of a shorter name than "without-compile-directory", but I can't. OTOH gdb uses "compilation directory" everywhere" $ grep "compile.*directory" *.[ch] | wc 0 ... $ grep "compilation.*directory" *.[ch] | wc 9 ... Can we change this to without-compilation-directory? [It's already long, and it still tab-completes in one character. :-)] In get_filename_display_from_sal: +const char * +get_filename_display_from_sal (struct symtab_and_line *sal) +{ + const char *filename =3D sal->symtab->filename; + const char *dirname =3D sal->symtab->dirname; + size_t dlen =3D dirname ? strlen (dirname) : 0; + + if (filename_display_string =3D=3D filename_display_basename + && filename) + { + return lbasename (filename); + } [...] There are a few "&& filename" checks. The code would be easier to read if you did if (filename =3D=3D NULL) return NULL; at the start. + else + if (filename_display_string =3D=3D filename_display_without_comp_directo= ry "else" and "if" go on the same line. In stack.c: diff -rup gdb-7.3.1-orig/gdb/stack.c gdb-7.3.1/gdb/stack.c --- gdb-7.3.1-orig/gdb/stack.c 2011-03-18 21:48:56.000000000 +0300 +++ gdb-7.3.1/gdb/stack.c 2011-10-30 20:53:23.182333185 +0400 @@ -835,11 +835,15 @@ print_frame (struct frame_info *frame, i ui_out_text (uiout, ")"); if (sal.symtab && sal.symtab->filename) { + const char *filename_display =3D get_filename_display_from_sal (&sal= ); + if (filename_display =3D=3D NULL) + filename_display =3D sal.symtab->filename; + annotate_frame_source_begin (); ui_out_wrap_hint (uiout, " "); ui_out_text (uiout, " at "); annotate_frame_source_file (); - ui_out_field_string (uiout, "file", sal.symtab->filename); + ui_out_field_string (uiout, "file", filename_display); if (ui_out_is_mi_like_p (uiout)) { const char *fullname =3D symtab_to_fullname (sal.symtab); If filename_display is NULL it's because sal.symtab->filename was NULL. [right?] This is confusing. I suggest removing this code: + if (filename_display =3D=3D NULL) + filename_display =3D sal.symtab->filename;