Closed Bug 1120257 Opened 8 years ago Closed 8 years ago

Clarify naming of inline strings


(Core :: JavaScript Engine, defect)

Not set





(Reporter: n.nethercote, Assigned: n.nethercote)



(2 files)

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 

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.
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.
Attachment #8547246 - Flags: review?(jdemooij)
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(-)
Attachment #8547247 - Flags: review?(jdemooij)
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.
Attachment #8547246 - Flags: review?(jdemooij) → review+
Comment on attachment 8547247 [details] [diff] [review]
(part 2) - Remove some duplicated inline string methods

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

Makes sense.
Attachment #8547247 - Flags: review?(jdemooij) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.