Thanks a lot for the review, Stan.

On Mon, Oct 10, 2011 at 8:34 AM, Stan Shebs <stanshebs@earthlink.net> wrote:
> On 10/6/11 4:15 PM, Justin Lebar wrote:

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

This ChangeLog process seems really archaic to me.  git log --stat
gives me much of this information already, and git blame gives me the
rest.  Keeping the ChangeLog in sync with my patch takes a significant
amount of work!  But anyway, I've fixed it up.

> +@kindex skip delete
> +@item skip delete @var{n}
> +Delete the skip with identifier @var{n}.
>
>
> No mass-delete by omitting the argument??

I've added mass- enable, disable, and delete.

> +  /* 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?
>
> 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.

What I have is, insofar as I can tell, exactly what we currently do
for breakpoints.

I can fake the length, but what about the call to ui_out_field_core_addr?

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

Yes, this is (finally) taken care of.

The attached patch addresses everything except this issue with
gdbarch.  I didn't rebase, so you should be able to use interdiff to
view the changes.

-Justin