Summary: | Support WTF::Vector pretty printer for GDB 7 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hayato Ito <hayato> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, evan, hamaji, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Hayato Ito
2010-06-21 05:01:36 PDT
Created attachment 59241 [details]
support-wtf-vector
Attachment 59241 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/gdb/wtf.py:41: expected 2 blank lines, found 1 [pep8/E302] [5]
WebKitTools/gdb/wtf.py:45: whitespace before '(' [pep8/E211] [5]
WebKitTools/gdb/wtf.py:92: indentation is not a multiple of four [pep8/E111] [5]
Total errors found: 3 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 59338 [details]
support-wtf-vector-fix-style
I've fixed the style. (In reply to comment #3) > Created an attachment (id=59338) [details] > support-wtf-vector-fix-style (In reply to comment #2) > Attachment 59241 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 > WebKitTools/gdb/wtf.py:41: expected 2 blank lines, found 1 [pep8/E302] [5] > WebKitTools/gdb/wtf.py:45: whitespace before '(' [pep8/E211] [5] > WebKitTools/gdb/wtf.py:92: indentation is not a multiple of four [pep8/E111] [5] > Total errors found: 3 in 2 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 59338 [details]
support-wtf-vector-fix-style
r- for minor issues.
WebKitTools/gdb/wtf.py:98
+ build_pritty_printers_dict()
pretty
WebKitTools/gdb/wtf.py:96
+ pretty_printers_dict = {}
It would be better not to use a global variable? Maybe
pretty_printers = build_pretty_printers()
gdb.pretty_printers.append(lambda val: lookup_function(pretty_printers, val))
(I'm not 100% sure if it's OK to pass lambda to gdb.pretty_printers though)
WebKitTools/gdb/wtf.py:43
+ "Print a WTF::Vector"
Could you add a document about how each members will be used? I guess both children and to_string will be called for GDB's p command when display_hint is 'array'? Maybe putting URL for GDB's python document would be nice.
WebKitTools/gdb/wtf.py:59
+ elt = self.item.dereference()
I think element would be better. Usually WebKit code doesn't like abbreviations.
Though I'm not good at GDB's python extension, I guess I will r+ the change in the next iteration as this change harms nothing and looks good to me. I'm CCing Evan as he could have some cool ideas to improve this patch. I also don't know a lot about GDB extensions, but the change looks good to me! You might want to mention it on webkit-dev once it's landed. Created attachment 62563 [details]
support-wtf-vector-typo-fixed-and-add-comments
Thank you for the review. (In reply to comment #5) > (From update of attachment 59338 [details]) > r- for minor issues. > > WebKitTools/gdb/wtf.py:98 > + build_pritty_printers_dict() > pretty Done. Thank you. > > WebKitTools/gdb/wtf.py:96 > + pretty_printers_dict = {} > It would be better not to use a global variable? Maybe > > pretty_printers = build_pretty_printers() > gdb.pretty_printers.append(lambda val: lookup_function(pretty_printers, val)) > > (I'm not 100% sure if it's OK to pass lambda to gdb.pretty_printers though) I don't think it is worth hiding 'pretty_printer_dict' from global scope. 'pretty_printer_dict' is global, but that is global only within this module. I'd like to leave that as is because that won't conflict any variables defined in other modules. > > > WebKitTools/gdb/wtf.py:43 > + "Print a WTF::Vector" > Could you add a document about how each members will be used? I guess both children and to_string will be called for GDB's p command when display_hint is 'array'? Maybe putting URL for GDB's python document would be nice. Sure. I've added comments mentioning the url and usage. > > WebKitTools/gdb/wtf.py:59 > + elt = self.item.dereference() > I think element would be better. Usually WebKit code doesn't like abbreviations. Done. Thank you. Comment on attachment 62563 [details] support-wtf-vector-typo-fixed-and-add-comments Looks good. Thanks for your update! > I don't think it is worth hiding 'pretty_printer_dict' from global scope. 'pretty_printer_dict' is global, but that is global only within this module. I'd like to leave that as is because that won't conflict any variables defined in other modules. I still like not to use global variables because I was a bit surprised this code seemed to use a variable without declaration. At first, I guessed pretty_printers_dict is something special in GDB extensions. But it's OK as is. (In reply to comment #7) > I also don't know a lot about GDB extensions, but the change looks good to me! You might want to mention it on webkit-dev once it's landed. Thanks for your comment! Comment on attachment 62563 [details] support-wtf-vector-typo-fixed-and-add-comments Clearing flags on attachment: 62563 Committed r64044: <http://trac.webkit.org/changeset/64044> All reviewed patches have been landed. Closing bug. |