Re: Status of 'blacklist' patch?

Stan Shebs
2011-10-10T12:34:27Z

On 10/6/11 4:15 PM, Justin Lebar wrote:
> There's a lot of change of terminology from "default breakpoint" to
> "displayed codepoint". I know we've debated better substitutes for
> "breakpoint", but this patch is maybe not the best place to introduce one.
> How about "last displayed symtab and line"? That seems to be
> something which is meaningful.

Or even "last displayed sal" :-)

+ * stack.h (clear_last_displayed_symtab_and_line,
+ last_displayed_symtab_and_line_is_valid, get_last_displayed_pspace,
+ get_last_displayed_addr, get_last_displayed_symtab,
+ get_last_displayed_line, get_last_displayed_symtab_and_line): Added
+

This is why we like periods at the end of each ChangeLog bit - this
looks like
it got cut off... plus it's good to say as "Declare." as reminder that it's
not a code bit that was added.

@@ -1394,7 +1397,7 @@ init_cli_cmds (void)
char *source_help_text;

/* Define the classes of commands.
- They will appear in the help list in the reverse of this order. */
+ They will appear in the help list in alphabetical order. */

I'm not going to be persnickety about this one, but we really want
random comment
fixups to be separate patches - took me a moment to decide if this was
somehow
relevant to the masses of code.

+Suppose you wish to step into the functions @code{foo} and @code{bar},
but you
+are not interested in stepping through @code{boring}. If you run
@code{step}
+at line 103, you'll enter @code{boring()}, but if you run @code{next},
you'll
+step over both @code{foo} and @code{boring}! What can you do?

I would lose the rhetorical question, it's not really adding much.

+
+@kindex skip delete
+@item skip delete @var{n}
+Delete the skip with identifier @var{n}.

No mass-delete by omitting the argument??

+
+ /* Architecture we used to create the skiplist entry. May be null
+ if the entry is pending a shared library load. */
+ struct gdbarch *gdbarch;

I'm not clear on why we need gdbarch, since CORE_ADDR should always be
long enough?

+ /* Count the number of rows in the table and see if we need space for a
+ 64-bit address anywhere. */
+ ALL_SKIPLIST_ENTRIES (e)
+ if (entry_num == -1 || e->number == entry_num)
+ {
+ num_printable_entries++;
+ if (e->gdbarch && gdbarch_addr_bit (e->gdbarch) > 32)
+ address_width = 18;
+ }

Ah, for address printing. My inclination is to say to drop this
admirable goal and make a separate patch that attempts to be smart for
address printing in breakpoint and skip lists in general. It seems like
a nice design might look at actual values in the list and only use wide
space if all addresses are large, sort of like how html table layout works.

+ ui_out_message (current_uiout, 0, _("Not ignoring any files or
functions.\n"));

"Not skipping"

In general, it's looking pretty good! As people have commented
previously, regexp would be nice to have, but with this much code, I
think it's better to get a first version in, accumulate a little
practical experience before deciding about which additional features to
add. (Between Moz and GCC, there are going to be lots of users I think.
:-) )

Also, in thumbing back through old discussion, I notice that the last
state was that you had submitted paperwork for copyright assignment, but
not received anything, and I don't remember getting any email adding you
to the "has assignments" list. Did you ever get the confirmation from
the FSF?

Stan
stan@codesourcery.com