Re: Status of 'blacklist' patch?

[Stan Shebs <stanshebs@earthlink.net>]

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