Closed Bug 1120257 Opened 10 years ago Closed 10 years ago

Clarify naming of inline strings

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

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

Details

Attachments

(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 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.
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+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: