Move *::dump() to GenericPrinter

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

We have many implementations of dump() that use FILE*. For unit testing purposes, it can be very useful to be able to dump to a string. Fortunately, we have GenericPrinter for this, so let's make use of it.
Blocks: 1377007
Comment hidden (mozreview-request)

Comment 2

3 months ago
mozreview-review
Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

https://reviewboard.mozilla.org/r/177306/#review182324

::: js/src/frontend/ParseNode.h:822
(Diff revision 1)
>          MOZ_ASSERT(NodeType::test(*this));
>          return *static_cast<const NodeType*>(this);
>      }
>  
>  #ifdef DEBUG
> -    void dump();
> +    void dump(GenericPrinter& out);

nit: The generic printer is not gdb friendly, add a dump() function which delegate to calling the with  "Fprinter out(stderr);"

::: js/src/jsobj.cpp:3724
(Diff revision 1)
>                              i.script(), i.script()->pcToOffset(i.pc()));
>          } else {
>              sprinter.printf(" (%p)\n", i.pc());
>          }
>      }
> -    fprintf(fp, "%s", sprinter.string());
> +    out.printf("%s", sprinter.string());

open a follow-up good first bug to replace sprinter by the generic printer in this function.

::: js/src/vm/Shape.cpp:1796
(Diff revision 1)
>          MOZ_ASSERT(JSID_IS_SYMBOL(propid));
> -        JSID_TO_SYMBOL(propid)->dump(fp);
> +        JSID_TO_SYMBOL(propid)->dump(out);
>      }
>  
> -    fprintf(fp, " g/s %p/%p slot %d attrs %x ",
> +    out.printf(" g/s %p/%p slot %d attrs %x ",
>              JS_FUNC_TO_DATA_PTR(void*, getter()),

nit: fix the indentation after out.printf new-lines.

::: js/src/vm/String.h:531
(Diff revision 1)
>      }
>  
>      static const JS::TraceKind TraceKind = JS::TraceKind::String;
>  
>  #ifdef DEBUG
> -    void dump(FILE* fp);
> +    void dump(js::GenericPrinter& out);

nit: add a dump() function which defaults to stderr.

::: js/src/vm/Symbol.h:98
(Diff revision 1)
>      size_t sizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const {
>          return mallocSizeOf(this);
>      }
>  
>  #ifdef DEBUG
> -    void dump(FILE* fp = stderr);
> +    void dump(js::GenericPrinter& out);

nit: add a dump() function, for gdb-friendly reasons.
Attachment #8905491 - Flags: review?(nicolas.b.pierron) → review+
Comment hidden (mozreview-request)
Blocks: 1397856

Comment 4

3 months ago
mozreview-review
Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

https://reviewboard.mozilla.org/r/177306/#review182452

::: js/src/frontend/ParseNode.h:1358
(Diff revisions 1 - 2)
> +// Debugger-friendly stderr printer.
>  void DumpParseTree(ParseNode* pn, GenericPrinter& out, int indent = 0);

How is the GenericPrinter& out debugger friendly?
Comment hidden (mozreview-request)
Assignee: nobody → dteller

Comment 6

3 months ago
mozreview-review
Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

https://reviewboard.mozilla.org/r/177306/#review182728

::: js/src/jsobj.cpp:3695
(Diff revisions 2 - 3)
> +JS_FRIEND_API(void)
>  js::DumpBacktrace(JSContext* cx, js::GenericPrinter& out)

Maybe we should keep all GenericPrinter functions out of the JS_FRIEND_API interface?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

3 months ago
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95bb61612572
Using GenericPrinter for DEBUG-only C++ dump() APIs;r=nbp
https://hg.mozilla.org/mozilla-central/rev/95bb61612572
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.