Minor improvements to pn->dump()

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
- default the indent parameter to 0
- spit out a newline at the end
- use pn_kind, not pn_op, to identify nodes
- for name nodes, spit out the actual name

This isn't enough to make pn->dump() a *great* tool; a lot of information is still just plain missing.

But then the parse trees are bad to begin with. Redundant information everywhere, obfuscatory optimization. Grrr.
(Assignee)

Comment 1

5 years ago
Created attachment 658192 [details] [diff] [review]
v1
Attachment #658192 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Assignee: general → jorendorff
(Assignee)

Comment 2

5 years ago
Uppercase city. Feel free to patch this to tolower() on output.

js> parse("function fib(x) { if (x <= 1) return x; else return fib(x-2)+fib(x-1); }")
(STATEMENTLIST [(FUNCTION (ARGSBODY [x
                                     (STATEMENTLIST [(IF (LE x
                                                             1)
                                                         (RETURN x)
                                                         (RETURN (ADD (CALL [fib
                                                                             (SUB x
                                                                                  2)])
                                                                      (CALL [fib
                                                                             (SUB x
                                                                                  1)]))))])]))])
Comment on attachment 658192 [details] [diff] [review]
v1

Review of attachment 658192 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/ParseNode.cpp
@@ +597,1 @@
>  

I suppose the changes here are basically just code-motion, so I didn't look too closely at them.

@@ +658,5 @@
> +      case PN_NAME:
> +        ((NameNode *) this)->dump(indent);
> +        break;
> +      default:
> +        fprintf(stderr, "?");

This seems...a bit quiet.  I would suggest an assert, but that kind of kills you in your tracks when you're in the middle of debugging, which seems a bit poor.  Maybe something screamy about a bad node?  I dunno.

@@ +668,5 @@
> +NullaryNode::dump()
> +{
> +    switch (getKind()) {
> +      case PNK_TRUE:  fprintf(stderr, "#true");  break;
> +      case PNK_FALSE: fprintf(stderr, "#false"); break;

This is a strange scheme.  Wouldn't #t and #f be more concise?

(Ignore this comment, it's all just to make a bad pun.)

@@ +686,5 @@
> +
> +      case PNK_STRING: {
> +        JSString::dumpChars(pn_atom->chars(), pn_atom->length());
> +        break;
> +      }

Braces aren't necessary here, I wouldn't add 'em.

@@ +787,5 @@
> +            DumpParseTree(expr(), indent + 2);
> +            fputc(')', stderr);
> +        }
> +        return;
> +    } 

Trailing whitespace.

::: js/src/frontend/ParseNode.h
@@ +175,5 @@
> +    F(LSH) \
> +    F(RSH) \
> +    F(URSH) \
> +    \
> +    /* Assignment operators (= += -= etc.). */ \

Probably we should comment on the ordering requirement for PNK_ASSIGNMENT_START/LAST.
Attachment #658192 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 4

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> > +        fprintf(stderr, "?");
> 
> This seems...a bit quiet.  I would suggest an assert, but that kind of kills
> you in your tracks when you're in the middle of debugging, which seems a bit
> poor.  Maybe something screamy about a bad node?  I dunno.

Changed to:
        fprintf(stderr, "#<BAD NODE %p, kind=%u, arity=%u>",
                (void *) this, unsigned(getKind()), unsigned(pn_arity));

> ::: js/src/frontend/ParseNode.h
> @@ +175,5 @@
> > +    F(LSH) \
> > +    F(RSH) \
> > +    F(URSH) \
> > +    \
> > +    /* Assignment operators (= += -= etc.). */ \
> 
> Probably we should comment on the ordering requirement for
> PNK_ASSIGNMENT_START/LAST.

Added:
    /* ParseNode::isAssignment assumes all these are consecutive. */

Surprisingly we don't make use of the ordering. We use pn_op. I'd like to get rid of pn_op; it's redundant with pn_kind in so many cases, and it stands to reason that having multiple "type" fields shouldn't be necessary.
https://hg.mozilla.org/mozilla-central/rev/1200e475363f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.