1063147 - Comments describing extensible strings are misleading

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Attachment: Clarify comments about extensible strings.

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Jim Blandy :jimb]

Attachment: Clarify comments about extensible strings. r=jandem

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.

Comment 3

[Jan de Mooij [:jandem]]

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.

Comment 4

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddae9d7a32c0

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ddae9d7a32c0