Status of 'blacklist' patch?
Tom Tromey
tromey@redhat.com
Thu Oct 20 19:46:00 GMT 2011
>>>>> "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
More information about the Gdb-patches
mailing list