This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Status of 'blacklist' patch?


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]