788227 - Minor improvements to pn->dump()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

- 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.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jason Orendorff [:jorendorff]]

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 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Jason Orendorff [:jorendorff]]

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

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1200e475363f