1485161 - gfx/thebes/gfxPlatform.cpp:1664:12: warning: local variable 'result' will be copied despite being returned by name [-Wreturn-std-move]

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement, P5)

Description

[Daniel Holbert [:dholbert]]

clang 8 build warning:
> gfx/thebes/gfxFontEntry.cpp:245:20: warning: local variable 'name'
>   will be copied despite being returned by name [-Wreturn-std-move]
>             return name;
>                    ^~~~
https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/gfx/thebes/gfxPlatform.cpp#1661-1672

This function uses a local nsAutoString, but we have to do allocation and string-copying to satisfy its "nsString" return type. (We have to create a nsString which doesn't get to bring along the nsAutoString's local buffer.)

But there's no reason this function has to return nsString -- in fact, this function only has two callers, and **both of them stick the return value in their own local nsAutoString variable! So we might as well just make it return nsAutoString, and then we can benefit from return value optimization and avoid the need for allocation/copying into a temporary nsString return value.

Comment 1

[Daniel Holbert [:dholbert]]

Sorry, I copypasted the wrong warning text into comment 0 -- meant to copy this one (matching my DXR link):
gfx/thebes/gfxPlatform.cpp:1664:12: warning: local variable 'result' will be copied despite being returned by name [-Wreturn-std-move]

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1485161: Make gfxPlatform::GetDefaultFontName() return nsAutoString, to enable possibility of Return Value Optimization & to address build warning. r=jfkthame

This function uses a nsAutoString internally, and its callers both store its
returned value in a nsAutoString. So it's silly for us to have it return a
different type (nsString). With this change, the compiler should be able to
perform return value optimization and avoid the need for any
copying/reallocation of this function's return value.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> But there's no reason this function has to return nsString -- in fact, this
> function only has two callers, and **both of them stick the return value in
> their own local nsAutoString variable!

Actually, I misread the DXR output -- there's only one caller (but it's a 3-line statement so it shows up as 3 results in DXR).

The one caller is here, for reference:
    nsAutoString defaultFontName(gfxPlatform::GetPlatform()->
        GetDefaultFontName(nsDependentCString(aLangGroup),
                           nsDependentCString(aGeneric)));

https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/gfx/src/nsThebesFontEnumerator.cpp#245

Here's a DXR search showing that this is the one caller:
https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22gfxPlatform%3A%3AGetDefaultFontName%28const+nsACString+%26%2C+const+nsACString+%26%29%22

(note that it shows 3 results, just because this is a 3-line statement)

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 9002936 [details]
Bug 1485161: Make gfxPlatform::GetDefaultFontName() return nsAutoString, to enable possibility of Return Value Optimization & to address build warning. r=jfkthame

Jonathan Kew (:jfkthame) has approved the revision.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/928dc169698a
Make gfxPlatform::GetDefaultFontName() return nsAutoString, to enable possibility of Return Value Optimization & to address build warning. r=jfkthame

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/928dc169698a