Refactor code to use StringBuffer

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 572320 [details] [diff] [review]
wip
(Assignee)

Updated

6 years ago
Assignee: general → evilpies
(Assignee)

Updated

6 years ago
Attachment #572320 - Attachment is patch: true
(Assignee)

Comment 1

6 years ago
Created attachment 572447 [details] [diff] [review]
wip 2
Attachment #572320 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Duplicate of this bug: 348545
(Assignee)

Comment 3

6 years ago
Created attachment 572592 [details] [diff] [review]
v1

I didn't touch obj_toSource, yet for obvious reasons.
Attachment #572592 - Flags: review?(jwalden+bmo)
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.
Attachment #572592 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 5

6 years ago
Created attachment 574315 [details] [diff] [review]
v2
Attachment #572447 - Attachment is obsolete: true
Attachment #572592 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
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 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.
(Assignee)

Updated

6 years ago
Attachment #574315 - Flags: review?(jwalden+bmo)
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.
Attachment #574315 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 579135 [details] [diff] [review]
v3

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

Updated

6 years ago
Blocks: 707911
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/35938622cde0
without the rewrite of StackTraceToString
No longer blocks: 707911
https://hg.mozilla.org/mozilla-central/rev/35938622cde0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

6 years ago
Blocks: 708231
You need to log in before you can comment on or make changes to this bug.