1489696 - xpcom/threads/Scheduler.cpp:798:10 [-Wreturn-std-move] local variable 'result' will be copied despite being returned by name

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Daniel Holbert [:dholbert]]

Build warning with clang trunk (8.0):
====
xpcom/threads/Scheduler.cpp:798:10 [-Wreturn-std-move] local variable 'result' will be copied despite being returned by name
====

This is a function that has a local variable nsPrintfCString, which it always returns, and its return-type is nsCString so we can't benefit from RVO.

Quoting the code:
> /* static */ nsCString
> Scheduler::GetPrefs()
> {
>   MOZ_ASSERT(XRE_IsParentProcess());
>   nsPrintfCString result("%d%d%d%d,%d",
>                          false, // XXX The scheduler is always disabled.
>                          Preferences::GetBool("dom.ipc.scheduler.chaoticScheduling",
>                                               SchedulerImpl::sPrefChaoticScheduling),
>                          Preferences::GetBool("dom.ipc.scheduler.preemption",
>                                               SchedulerImpl::sPrefPreemption),
>                          Preferences::GetBool("dom.ipc.scheduler.useMultipleQueues",
>                                               SchedulerImpl::sPrefUseMultipleQueues),
>                          Preferences::GetInt("dom.ipc.scheduler.threadCount",
>                                              SchedulerImpl::sPrefThreadCount));
> 
>   return result;
> }
https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/xpcom/threads/Scheduler.cpp#783

Looks like the string is quite short -- something like "1010,5678" (bool bool bool bool, int).  So it'll always fit in nsPrintfCString's stack-allocated buffer, and the fact that we're shoehorning it into a nsCString means we're doing heap allocation and string-copying unnecessarily.

Happily, this function has only 1 caller, which doesn't particularly care about the underlying string type -- it just calls ".get()" on the string:
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2224

So given that, let's change the return value to match the local variable, to avoid the heap allocation, string copying, and build warning.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1489696: Adjust Scheduler::GetPrefs() return type to let it benefit from RVO and to address -Wreturn-std-move build warning. r=froydnj

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 9007396 [details]
Bug 1489696: Adjust Scheduler::GetPrefs() return type to let it benefit from RVO and to address -Wreturn-std-move build warning. r=froydnj

Nathan Froyd [:froydnj] has approved the revision.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04c0f2151fe9
Adjust Scheduler::GetPrefs() return type to let it benefit from RVO and to address -Wreturn-std-move build warning. r=froydnj

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/04c0f2151fe9