Closed Bug 1485161 Opened Last year Closed Last year

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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]
Summary: gfx/thebes/gfxFontEntry.cpp:245:20: warning: local variable 'name' will be copied despite being returned by name [-Wreturn-std-move] → gfx/thebes/gfxPlatform.cpp:1664:12: warning: local variable 'result' will be copied despite being returned by name [-Wreturn-std-move]
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.
(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 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.
Attachment #9002936 - Flags: review+
Pushed by dholbert@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/928dc169698a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.