Re: Status of 'blacklist' patch?

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Justin" == Justin Lebar <justin.lebar@gmail.com> writes:

Justin> Added a NEWS entry in this patch.

Thanks.

I think this still needs a doc review.

Some review below.  I think this patch is pretty close to being ready;
and I am eager for it to go in.

Justin> +  /* null if this isn't a skiplist entry for an entire file.

"NULL"

Justin> +     The skiplist entry owns this pointer. */

In our style, sentences have 2 spaces after the period.

Justin> +//static void try_resolve_pending_entry (struct skiplist_entry *e);

We can't use "//" comments in gdb, but I think there's really no reason
for a new file to have commented-out code.

Justin> +      TRY_CATCH(decode_exception, NOT_FOUND_ERROR)

Space before the open paren.

Justin> +  if (arg != 0)
Justin> +    {
Justin> +      entry_num = parse_and_eval_long (arg);
Justin> +    }

This will parse and evaluate an expression.  I think something like what
"info break" does would be more in keeping with other places in gdb --
that is, use get_number_or_range, then iterate.

Justin> +      entry_chain = make_cleanup_ui_out_tuple_begin_end (current_uiout, "blklst-entry");

I think this line is too long.
Maybe a few others in this function, too.

Justin> +static void
Justin> +skip_enable_command (char *arg, int from_tty)
Justin> +{
Justin> +  struct skiplist_entry *e;
Justin> +  int entry_num;
Justin> +  if (arg == 0)

Newline after the declaration block, here and elsewhere.

Justin> +  entry_num = parse_and_eval_long (arg);

This command and others should use get_number_or_range.

Justin> +      ALL_SKIPLIST_ENTRIES_SAFE(e, temp)

Space before open paren.

Justin> +  ALL_SKIPLIST_ENTRIES (e)
Justin> +    {
Justin> +      int pc_match = e->pc != 0 && pc == e->pc;
Justin> +      int filename_match = e->filename != 0 && filename != 0 &&

You have to parenthesize the RHS according to GNU standards, and put the
"&&" at the start of the second line.

Justin> +			   strcmp (filename, e->filename) == 0;
Justin> +      if (e->enabled && !e->pending && (pc_match || filename_match))
Justin> +	return 1;

I think you could check enabled and pending earlier and avoid the strcmp
unless truly needed.

Also I think it would be worthwhile to lazily compute the SAL in this
function, to avoid computing it at all when there are no (or no enabled)
skip entries.

Justin> +void
Justin> +skip_re_set ()

Should be '(void)'

Justin> +	  TRY_CATCH(decode_exception, NOT_FOUND_ERROR)

Space before paren.

Justin> +	  if (decode_exception.reason >= 0 &&

"&&" on next line.

Justin> +                  e->function_name = xstrdup(func_name);

Space before paren.

Justin> +/* Helper function to get a gdbarch from a symtab_and_line. */
Justin> +static struct gdbarch*
Justin> +get_sal_arch (struct symtab_and_line *sal)
Justin> +{
Justin> +  if (sal->section)
Justin> +    return get_objfile_arch (sal->section->objfile);
Justin> +  if (sal->symtab)
Justin> +    return get_objfile_arch (sal->symtab->objfile);
Justin> +  return get_current_arch ();
Justin> +}

I think it is better to make this public in breakpoint.c.

Really, we should probably have a "sal.h" and "sal.c" and treat it like
a real data structure.  Not your job though.

Justin> +  add_prefix_cmd ("skip", class_breakpoint, skip_function_command, _("\
Justin> +Ignore a function while stepping.\n\
Justin> +skip [FUNCTION NAME]\n\

I'd like this line to start with "Usage: " (and likewise in the other
help text).

Justin> +void skip_re_set ();

Should be '(void)'.

Justin> +struct program_space*
Justin> +get_last_displayed_pspace ()

Space before "*".
"(void)", here and elsewhere.

Most of the functions around this need introductory comments.

Justin> +void clear_last_displayed_sal ();
Justin> +int last_displayed_sal_is_valid ();
Justin> +struct program_space* get_last_displayed_pspace ();
Justin> +CORE_ADDR get_last_displayed_addr ();
Justin> +struct symtab* get_last_displayed_symtab ();
Justin> +int get_last_displayed_line ();

"(void)" for all of these.

Tom