Closed Bug 1489696 Opened Last year Closed Last year

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

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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.
Attachment #9007396 - Flags: review+
Pushed by dholbert@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/04c0f2151fe9
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: nobody → dholbert
You need to log in before you can comment on or make changes to this bug.