1120257 - Clarify naming of inline strings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Currently we have JSInlineString and JSFatInlineString, and the latter is a 
subclass of the former. This naming is confusing because if you say 
JSInlineString or "inline string" it's unclear if you're including the fat 
kind.

This leads to things like the functions AllocateFatInlineString() and
NewFatInlineString() and ConcatFatInlineString() which actually allocate a
non-fat inline string if possible, and return a JSInlineString pointer. I have
been misled by these function names several times.

These problems would be avoided if we have distinct names for (a) fat inline
strings, (b) non-fat inline strings, and (c) all inline strings.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Clarify inline string naming by adding JSThinInline

This patch introduces a new sub-class of JSInlineString called
JSThinInlineString, and moves the operations specific to thin inline strings
into it. JSInlineString now clearly subsumes both of JS{Thin,Fat}InlineString,
and "inling string" now clearly subsumes both of "thin inline string" and "fat
inline string". Despite adding yet another JSString subclass, I think it makes
things clearer :)

The patch also renames a bunch of things accordingly, and removes the dead
resetLength() declaration.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove some duplicated inline string methods

Some of the inline string methods have a templated and a non-templated version,
e.g. lengthFits() and init(). Others don't, e.g. NewInlineString(). Having both
seems unnecessary, so this patch removes the non-templated versions.

 4 files changed, 75 insertions(+), 136 deletions(-)

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8547246 [details] [diff] [review]
(part 1) - Clarify inline string naming by adding JSThinInline

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

Nice, this is less confusing.

::: js/src/vm/String-inl.h
@@ +70,2 @@
>  {
>      MOZ_ASSERT(JSFatInlineString::lengthFits<CharT>(length));

Nit: use JSInlineString here as well to match AllocateInlineString.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8547247 [details] [diff] [review]
(part 2) - Remove some duplicated inline string methods

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

Makes sense.

Comment 5

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59f05d35d9b7

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/270cd86a7e4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f867a45b8068

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/270cd86a7e4c
https://hg.mozilla.org/mozilla-central/rev/f867a45b8068