Various "-Wreturn-std-move" build warnings with clang 7.0 (trunk), for cases where return invokes (cheap) string copy-constructor rather than move constructor

RESOLVED DUPLICATE of bug 1453990

Status

()

RESOLVED DUPLICATE of bug 1453990
11 months ago
9 months ago

People

(Reporter: dholbert, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox61 affected)

Details

(Reporter)

Description

11 months ago
Clang trunk (version 7.0) gives me the following 17 warnings:

> dom/media/ChannelMediaDecoder.cpp:625:10 [-Wreturn-std-move] local variable 'str' will be copied despite being returned by name
> dom/media/MediaDecoderStateMachine.cpp:3809:10 [-Wreturn-std-move] local variable 'str' will be copied despite being returned by name
> dom/media/mediasink/AudioSinkWrapper.cpp:261:10 [-Wreturn-std-move] local variable 'str' will be copied despite being returned by name
> dom/media/mediasink/DecodedStream.cpp:805:10 [-Wreturn-std-move] local variable 'str' will be copied despite being returned by name
> dom/media/mediasink/VideoSink.cpp:566:10 [-Wreturn-std-move] local variable 'str' will be copied despite being returned by name
> dom/storage/LocalStorageManager.cpp:145:10 [-Wreturn-std-move] local variable 'scope' will be copied despite being returned by name
> gfx/thebes/gfxFontEntry.cpp:258:20 [-Wreturn-std-move] local variable 'name' will be copied despite being returned by name
> layout/painting/DisplayItemClip.cpp:452:10 [-Wreturn-std-move] local variable 'str' will be copied despite being returned by name
> layout/painting/DisplayItemClipChain.cpp:82:10 [-Wreturn-std-move] local variable 'str' will be copied despite being returned by name
> layout/painting/nsDisplayList.cpp:158:10 [-Wreturn-std-move] local variable 'str' will be copied despite being returned by name
> toolkit/components/extensions/WebExtensionPolicy.cpp:379:12 [-Wreturn-std-move] local variable 'result' will be copied despite being returned by name
> toolkit/components/extensions/WebExtensionPolicy.cpp:392:10 [-Wreturn-std-move] local variable 'result' will be copied despite being returned by name
> toolkit/components/url-classifier/LookupCache.h:61:12 [-Wreturn-std-move] local variable 'hex' will be copied despite being returned by name
> xpcom/glue/FileUtils.cpp:187:10 [-Wreturn-std-move] local variable 'libname' will be copied despite being returned by name
> xpcom/glue/FileUtils.cpp:217:10 [-Wreturn-std-move] local variable 'path' will be copied despite being returned by name
> xpcom/threads/Scheduler.cpp:792:10 [-Wreturn-std-move] local variable 'result' will be copied despite being returned by name
> obj/dist/include/LookupCache.h:61:12 [-Wreturn-std-move] local variable 'hex' will be copied despite being returned by name

I haven't looked at all of them, but from a quick sampling, it looks like they're all cases where a function has return type "nsString", and we have a return statement with some other type, e.g. "nsAutoString".  (or similar with nsCString / nsAutoCString).  In some cases we're using nsPrintfCString instead of the "auto" flavor.


I think clang is worried that we're invoking the copy constructor when we *could* be invoking the move constructor. And it's right to be worried, in general, but in this case, IIRC our string copy-constructors are typically just as efficient as their move-constructors, because they share data where possible (and I believe neither constructor shares data where it's impossible, e.g. for a nsAutoString's stack-allocated pointer).

Nonetheless, we don't have a ton of these warnings -- only 17 that I see -- so perhaps we should just fix all these callsites so that they don't create problems in warnings-as-errors builds down the road.
(Reporter)

Comment 1

11 months ago
It doesn't matter too much, but for reference, I think the copy constructor (with single &) and move constructor (double &&) for these string types are defined here:
=========
  explicit
  nsTString(const substring_type& aReadable)
    : substring_type(ClassFlags::NULL_TERMINATED)
  {
    this->Assign(aReadable);
  }

  explicit
  nsTString(substring_type&& aReadable)
    : substring_type(ClassFlags::NULL_TERMINATED)
  {
    this->Assign(mozilla::Move(aReadable));
  }
=========
https://dxr.mozilla.org/mozilla-central/rev/0ceabd10aac2272e83850e278c7876f32dbae42e/xpcom/string/nsTString.h#112-124

(I think "substring_type" there subsumes the various types-to-be-copied here -- nsAutoString, nsPrintfCString, etc.)
I've also seen this in other places in our code, see bug 1453990. But, at least, in the bug that I mention I don't think we should do the std:move. I think by doing this we instruct the compiler to just do an RValue cast and omit the possibility of having RVO. Sure moving is much cost effective than rvalue binding, but I don't think it's better than ROV, at least where it applies. I also found this interesting article from IBM: https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/RVO_V_S_std_move?lang=en that compares the performance between Rvalue return binding and RVO, and their preliminary conclusion is that RVO is more cost effective.
See Also: → bug 1453990
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1453990
(Reporter)

Updated

9 months ago
See Also: bug 1453990
You need to log in before you can comment on or make changes to this bug.