Bug 40909 – Support WTF::Vector pretty printer for GDB 7

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hayato Ito]

I noticed that there is a WebKitTools/gdb/webcore.py.
It would be better to support more types for GDB lovers.

[Hayato Ito]

Created attachment 59241 [details]
support-wtf-vector

[WebKit Review Bot]

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.

[Hayato Ito]

Created attachment 59338 [details]
support-wtf-vector-fix-style

[Hayato Ito]

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.

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

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.

[Evan Martin]

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.

[Hayato Ito]

Created attachment 62563 [details]
support-wtf-vector-typo-fixed-and-add-comments

[Hayato Ito]

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.

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

(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!

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.