700169 - Refactor code to use StringBuffer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Attachment: wip

Comment 1

[Tom S [:evilpie]]

Attachment: wip 2

Comment 3

[Tom S [:evilpie]]

Attachment: v1

I didn't touch obj_toSource, yet for obvious reasons.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 572592 [details] [diff] [review]
v1

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

There are enough changes needed to the error code that I'd like a second look, but even the cleanup thus far is immensely satisfying.  -120 lines of horrible, grody code in jsexn.cpp, egad!

::: js/src/jsatom.h
@@ +356,5 @@
>  
>      js::PropertyName    *returnAtom;
>      js::PropertyName    *throwAtom;
>  
> +    js::PropertyName    *ErrorAtom;

Don't add this -- use JSAtomState::classAtoms[JSProto_Error] instead, which is most easily accessed via CLASS_ATOM(cx, Error) (macro, ugh).

::: js/src/jsexn.cpp
@@ +604,4 @@
>          }
> +        if (!sb.append(':') || !NumberValueToStringBuffer(cx, NumberValue(element->ulineno), sb) 
> +            || !sb.append('\n'))
> +            return NULL;

Brace the if body (see above or below, whichever direction I commented on this).

@@ +705,5 @@
>   * is left to the host to check for private data and report filename and line
>   * number information along with this message.
>   */
>  static JSBool
>  exn_toString(JSContext *cx, uintN argc, Value *vp)

I was confused about the steps you were implementing here, reading the spec, until I realized you were implementing the spec plus errata.  It would be helpful if that were clarified.

Speaking of which, the comment above this shouldn't exist any more -- it should be a normative spec reference.  So let's replace the entire comment with this, killing two birds with one stone:

/* ES5 15.11.4.4 (NB: with subsequent errata). */

@@ +707,5 @@
>   */
>  static JSBool
>  exn_toString(JSContext *cx, uintN argc, Value *vp)
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);

The point of CallArgs is that you use argc/vp only to create one, then you never touch those variables.  This method intermixes both: it should use only CallArgs.

@@ +708,5 @@
>  static JSBool
>  exn_toString(JSContext *cx, uintN argc, Value *vp)
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JSString *name, *message;

These should move down to be declared as close to first use as possible.  This positioning is a holdover from the bad old C days, and we remove it whenever we're touching code that uses it.

@@ +719,5 @@
>  
> +    /* Step 1. */
> +    JSObject *obj = &args.thisv().toObject();
> +    if (!obj)
> +        return false;

JSObject &obj = args.thisv().toObject();

isObject() means is an object and is not null.  Using a JSObject reference type clarifies this.

@@ +722,5 @@
> +    if (!obj)
> +        return false;
> +
> +    /* Step 3. */
> +    if (!obj->getProperty(cx, cx->runtime->atomState.nameAtom, vp))

vp mixing.  Use a Value nameVal on the stack here.

@@ +727,5 @@
> +        return false;
> +    /* todo: there was an error involving empty name or undefined name */
> +    /* Step 4. */
> +    if (vp->isUndefined()) {
> +        name = cx->runtime->atomState.ErrorAtom;

|name| should be a JSString*.

@@ +732,5 @@
> +    } else {
> +        name = js_ValueToString(cx, *vp);
> +        if (!name)
> +            return false;
> +        vp->setString(name);

No need to do this, it'll be rooted by the conservative stack scanner until it's finally needed.

@@ +734,5 @@
> +        if (!name)
> +            return false;
> +        vp->setString(name);
> +    }
> +    /* Step 5. */

Add blank lines between steps of the algorithm.  This applies to every step here, not just step 5.

@@ +735,5 @@
> +            return false;
> +        vp->setString(name);
> +    }
> +    /* Step 5. */
> +    AutoValueRooter msg(cx);

It's no longer necessary to use AutoValueRooter here -- the conservative stack scanner will keep the value alive.

@@ +748,5 @@
> +            return false;
> +        msg.set(StringValue(message));
> +    }
> +    /* Step 7. */
> +    if (name->length() == 0 && message->length() == 0) {

name->empty() && message->empty()

@@ +749,5 @@
> +        msg.set(StringValue(message));
> +    }
> +    /* Step 7. */
> +    if (name->length() == 0 && message->length() == 0) {
> +        args.rval().setString(cx->runtime->atomState.ErrorAtom);

CLASS_ATOM(cx, Error)

@@ +753,5 @@
> +        args.rval().setString(cx->runtime->atomState.ErrorAtom);
> +        return true;
> +    }
> +    /* Step 8. */
> +    if (name->length() == 0) {

name->empty()

@@ +758,5 @@
> +        args.rval().setString(message);
> +        return true;
> +    }
> +    /* Step 9. */
> +    if (message->length() == 0) {

message->empty()

@@ +801,1 @@
>          AutoArrayRooter tvr(cx, ArrayLength(localroots), localroots);

We no longer need localroots, either, due to conservative stack scanning.  Make each of these individual things a Value on the stack.

::: js/src/jsnum.cpp
@@ +607,5 @@
>      if (!BoxedPrimitiveMethodGuard(cx, args, num_toSource, &d, &ok))
>          return ok;
>  
> +    StringBuffer sb(cx);
> +    if (!sb.append("(new Number(") || !NumberValueToStringBuffer(cx, NumberValue(d), sb) ||

It might possibly be worthwhile to have IntegerToStringBuffer and DoubleToStringBuffer (int32_t and double) and have callers make that distinction, since sometimes they can, and when they can't it's quick work to do so.  Just musing, no need to do anything here/now.

@@ +613,1 @@
>          return false;

If the condition of an if spans multiple lines, brace the body of the if.

::: js/src/jsobj.cpp
@@ +89,5 @@
>  #include "jsinterpinlines.h"
>  #include "jsscopeinlines.h"
>  #include "jsscriptinlines.h"
>  #include "jsobjinlines.h"
> +#include "jsstrinlines.h"

This isn't in alphabetical order -- make it so.

Comment 5

[Tom S [:evilpie]]

Attachment: v2

Comment 6

[Tom S [:evilpie]]

Comment on attachment 572592 [details] [diff] [review]
v1

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

Ups, forgot the publish this before I attached the new patch.

::: js/src/jsexn.cpp
@@ +727,5 @@
> +        return false;
> +    /* todo: there was an error involving empty name or undefined name */
> +    /* Step 4. */
> +    if (vp->isUndefined()) {
> +        name = cx->runtime->atomState.ErrorAtom;

Not sure what you want? Should I cast here?

::: js/src/jsnum.cpp
@@ +607,5 @@
>      if (!BoxedPrimitiveMethodGuard(cx, args, num_toSource, &d, &ok))
>          return ok;
>  
> +    StringBuffer sb(cx);
> +    if (!sb.append("(new Number(") || !NumberValueToStringBuffer(cx, NumberValue(d), sb) ||

OK going to fill a bug for that.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 572592 [details] [diff] [review]
v1

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

::: js/src/jsexn.cpp
@@ +727,5 @@
> +        return false;
> +    /* todo: there was an error involving empty name or undefined name */
> +    /* Step 4. */
> +    if (vp->isUndefined()) {
> +        name = cx->runtime->atomState.ErrorAtom;

I'm not sure what I meant here either.  :-)  Ignore this.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 574315 [details] [diff] [review]
v2

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

I *so* want this cleanup.  r=me with nits fixed.

::: js/src/jsdate.cpp
@@ +2486,1 @@
>          return false;

This should be braced, if the condition overflows a line.  Note that bracing style for multiline ifs is now this:

  if (...
      ...)
  {
     ...
  }

...because otherwise the condition and single-statement body line up too perfectly, and it's hard to distinguish one from the other.

::: js/src/jsexn.cpp
@@ +607,3 @@
>          }
> +        if (!sb.append(':') || !NumberValueToStringBuffer(cx, NumberValue(element->ulineno), sb) 
> +            || !sb.append('\n')) {

Brace should be on a new line, per new style.

@@ +800,5 @@
>  
> +    Value messageVal;
> +    JSString *message;
> +    if (!obj->getProperty(cx, cx->runtime->atomState.messageAtom, &messageVal) ||
> +        !(message = js_ValueToSource(cx, messageVal))) {

Brace on a new line, while you're touching this.  Also do this for the rest of the similar-looking ifs in this function.

::: js/src/jsnum.cpp
@@ +608,5 @@
>          return ok;
>  
> +    StringBuffer sb(cx);
> +    if (!sb.append("(new Number(") || !NumberValueToStringBuffer(cx, NumberValue(d), sb) ||
> +        !sb.append("))")) {

Brace on new line.

::: js/src/jsobj.cpp
@@ +860,1 @@
>          return NULL;

Bracing.

Comment 9

[Tom S [:evilpie]]

Attachment: v3

Nits resolved, and I put back the stack trace limit in StackTraceToString, so we don't OOM.

Comment 10

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/35938622cde0
without the rewrite of StackTraceToString

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/35938622cde0