Re: Status of 'blacklist' patch?

[Justin Lebar <justin.lebar@gmail.com>]

> 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.

breakpoint_1 calls parse_and_eval_long:

	  if (allflag && parse_and_eval_long (args) != b->number)
	    continue;
	  if (!allflag && !number_is_in_list (args, b->number))
	    continue;

Do we want to copy the breakpoint code here, or not?

On Thu, Oct 20, 2011 at 3:33 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "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
>