Closed
Bug 1120257
Opened 10 years ago
Closed 10 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•10 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•10 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•10 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•10 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•10 years ago
|
||
![]() |
Assignee | |
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/270cd86a7e4c
https://hg.mozilla.org/mozilla-central/rev/f867a45b8068
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.
Description
•