Closed Bug 1260570 Opened 4 years ago Closed 4 years ago

Common up ScriptSource creation and the get-or-create'ing of SharedImmutableTwoByteStrings

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1211723

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file)

From bug 1211723 comment 60:

> @@ +1797,5 @@
> > +    mozilla::UniquePtr<char16_t[], JS::FreePolicy> ownedSource(src);
> > +    auto& cache = cx->runtime()->sharedImmutableStrings();
> > +    auto deduped = cache.getOrCreate(mozilla::Move(ownedSource), length);
> > +    if (!deduped) {
> > +        ReportOutOfMemory(cx);
> 
> We're introducing this boilerplate in several places, as we lift up the
> de-duplication to the higher level. It's never nice to see call sites being
> made more complicated. Would it be possible to give ScriptSource its own
> methods that take care of this?
Comment on attachment 8747929 [details] [diff] [review]
Common up getting a de-duplicated string to set as a ScriptSource's text

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

::: js/src/jsscript.cpp
@@ +1796,5 @@
>      if (!cx->runtime()->sourceHook->load(cx, ss->filename(), &src, &length))
>          return false;
>      if (!src)
>          return true;
> +    if (!ss->setSource(cx, mozilla::UniquePtr<char16_t[], JS::FreePolicy>(src), length))

Checking my understanding: There's no need for a `Move` here because it's the result of calling a function, so it's automatically an rvalue.
Attachment #8747929 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #2)
> ::: js/src/jsscript.cpp
> @@ +1796,5 @@
> >      if (!cx->runtime()->sourceHook->load(cx, ss->filename(), &src, &length))
> >          return false;
> >      if (!src)
> >          return true;
> > +    if (!ss->setSource(cx, mozilla::UniquePtr<char16_t[], JS::FreePolicy>(src), length))
> 
> Checking my understanding: There's no need for a `Move` here because it's
> the result of calling a function, so it's automatically an rvalue.

That is correct.
(In reply to Jim Blandy :jimb from comment #2)
> There's no need for a `Move` here because it's
> the result of calling a function, so it's automatically an rvalue.

Somewhat more precisely, it's a temporary value, ergo an rvalue.  A function could return a reference (precisely, an rvalue reference, e.g. T&), and that wouldn't be an rvalue, and so would require Move.
Fixed bustage in blocking bug's patch, and here's a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ed125fdef88
Folded this bug into bug 1211723's patch because rebasing them as a whole was easier.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1211723
You need to log in before you can comment on or make changes to this bug.