Closed Bug 1454848 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1453990
Tracking Status
firefox61 --- affected

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

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.
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: → 1453990
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
See Also: 1453990
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.