Closed
Bug 1120257
Opened 9 years ago
Closed 9 years ago
Clarify naming of inline strings
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(2 files)
33.05 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
14.43 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59f05d35d9b7
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/270cd86a7e4c https://hg.mozilla.org/integration/mozilla-inbound/rev/f867a45b8068
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/270cd86a7e4c https://hg.mozilla.org/mozilla-central/rev/f867a45b8068
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•