Closed Bug 1063147 Opened 7 years ago Closed 6 years ago

Comments describing extensible strings are misleading

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #8484516 - Flags: review?(jdemooij)
Comment on attachment 8484516 [details] [diff] [review]
Clarify comments about extensible strings.

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

Awesome, thanks for fixing those comments. r=me with nits below addressed.

::: js/src/vm/String.cpp
@@ +324,5 @@
>       *
> +     * To do this, we compute the size of the flattened text, round it up, and
> +     * then flatten the rope into an extensible string that has that much extra
> +     * buffer space at the end. (Extensible strings record both their length and
> +     * their buffer's available capacity.) Then, if the resulting extensible

As far as I can see, it's not the available capacity but the *full* capacity (used + unused space).

::: js/src/vm/String.h
@@ +81,5 @@
>   *  - To avoid allocating small char arrays, short strings can be stored inline
>   *    in the string header (JSInlineString). To increase the max size of such
>   *    inline strings, larger string headers can be used (JSFatInlineString).
>   *
>   *  - To avoid comparing O(n) string equality comparison, strings can be

Unrelated, but can you also remove the "Note that Latin1 strings are not yet enabled by default, see bug 998392." sentence from this comment while you're here? I enabled it in 33 but forgot to update the comment.

@@ +112,5 @@
>   * JSFlatString                 - / null terminated
>   *  |  |
>   *  |  +-- JSExternalString     - / char array memory managed by embedding
>   *  |  |
> + *  |  +-- JSExtensibleString   tracks buffer capacity after actual text

Same here, it includes the actual text.
Attachment #8484516 - Flags: review?(jdemooij) → review+
Based on experiments with the dumpStringRepresentation function added in bug 1144371, I've revised these comments. Since I've made major changes since the September version, asking for re-review.
Assignee: nobody → jimb
Attachment #8484516 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8578991 - Flags: review?(jdemooij)
Comment on attachment 8578991 [details] [diff] [review]
Clarify comments about extensible strings. r=jandem

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

Looks good, great comments.
Attachment #8578991 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddae9d7a32c0
Flags: in-testsuite-
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.