Closed Bug 1397717 Opened 6 years ago Closed 6 years ago

Move *::dump() to GenericPrinter

Categories

(Core :: JavaScript Engine, enhancement)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file)

Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;
59 bytes, text/x-review-board-request
nbp
: review+
Details 
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.
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 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?
Assignee: nobody → dteller
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?
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
Closed: 6 years 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.