Re: [patch] GDB 7.2: new feature for "backtrace" that cuts path to file (remain filename)

[Phil Muldoon <pmuldoon@redhat.com>]

iam ahal <hal9000ed2k@gmail.com> 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 <hal9000ed2k@gmail.com>:

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