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

[Doug Evans <dje@google.com>]

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;