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

Doug Evans
2011-11-02T22:53:44Z

On Sun, Oct 30, 2011 at 11:16 AM, iam ahal <hal9000ed2k@gmail.com> 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:
* gdb.texinfo (Values From Inferior): Add is_lazy attribute,
fetch_lazy method.

For this entry:

* frame.c: Added including of a header file.  Added 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:
* 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:
* 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 = sal->symtab->filename;
+ const char *dirname = sal->symtab->dirname;
+ size_t dlen = dirname ? strlen (dirname) : 0;
+
+ if (filename_display_string == 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 == NULL)
return NULL;

at the start.

+ else
+ if (filename_display_string == filename_display_without_comp_directory

"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 = get_filename_display_from_sal (&sal);
+ if (filename_display == NULL)
+ filename_display = 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 = 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 == NULL)
+ filename_display = sal.symtab->filename;