Closed Bug 1457137 Opened 2 years ago Closed 2 years ago

Fix clang 6 warnings / slightly optimize HTMLMediaElement::MozDumpDebugInfo

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: padenot, Assigned: padenot)

Details

Attachments

(1 file)

No description provided.
Rank: 29
Priority: -- → P3
Comment on attachment 8971209 [details]
Bug 1457137 - Move instead of copying strings returned from various GetDebugInfo functions for HTMLMediaElement::MozDumpDebugInfo.

https://reviewboard.mozilla.org/r/239994/#review245764

There's no need to move return value.
Actually, it's always better to not move, as otherwise you prohibit copy elision

::: dom/media/ChannelMediaDecoder.cpp:625
(Diff revision 1)
>  {
>    auto&& str = MediaDecoder::GetDebugInfo();
>    if (mResource) {
>      AppendStringIfNotEmpty(str, mResource->GetDebugInfo());
>    }
> -  return str;
> +  return std::move(str);

Use Move(str)

::: dom/media/MediaDecoderStateMachine.cpp:3809
(Diff revision 1)
>      mVideoCompleted,
>      mStateObj->GetDebugInfo().get());
>  
>    AppendStringIfNotEmpty(str, mMediaSink->GetDebugInfo());
>  
> -  return str;
> +  return std::move(str);

same

::: dom/media/MediaDecoderStateMachine.cpp:3809
(Diff revision 1)
>      mVideoCompleted,
>      mStateObj->GetDebugInfo().get());
>  
>    AppendStringIfNotEmpty(str, mMediaSink->GetDebugInfo());
>  
> -  return str;
> +  return std::move(str);

this isn't necessary, move semantic here is automatic. and should be left up to the compiler

::: dom/media/mediasink/AudioSinkWrapper.cpp:261
(Diff revision 1)
>                      IsPlaying(),
>                      mAudioEnded);
>    if (mAudioSink) {
>      AppendStringIfNotEmpty(str, mAudioSink->GetDebugInfo());
>    }
> -  return str;
> +  return std::move(str);

same

::: dom/media/mediasink/DecodedStream.cpp:805
(Diff revision 1)
>                      mPlaying,
>                      mData.get());
>    if (mData) {
>      AppendStringIfNotEmpty(str, mData->GetDebugInfo());
>    }
> -  return str;
> +  return std::move(str);

same

::: dom/media/mediasink/VideoSink.cpp:566
(Diff revision 1)
>      mVideoFrameEndTime.ToMicroseconds(),
>      mHasVideo,
>      mVideoSinkEndRequest.Exists(),
>      mEndPromiseHolder.IsEmpty());
>    AppendStringIfNotEmpty(str, mAudioSink->GetDebugInfo());
> -  return str;
> +  return std::move(str);

same
Attachment #8971209 - Flags: review?(jyavenard) → review-
Attachment #8971209 - Flags: review?(gsquelart)
Comment on attachment 8971209 [details]
Bug 1457137 - Move instead of copying strings returned from various GetDebugInfo functions for HTMLMediaElement::MozDumpDebugInfo.

https://reviewboard.mozilla.org/r/239994/#review245764

turned out, the object returned is a nsPrintfCString, as such automatic move isn't working
nsPrintfCString
Attachment #8971209 - Flags: review?(jyavenard) → review?(gsquelart)
gerald, this [0] is how I found the issue (using a recent clang with this warning), and I think the thread is enlightning.

[0]: https://reviews.llvm.org/D43322
Comment on attachment 8971209 [details]
Bug 1457137 - Move instead of copying strings returned from various GetDebugInfo functions for HTMLMediaElement::MozDumpDebugInfo.

https://reviewboard.mozilla.org/r/239994/#review245984

r+ without the Move in ChannelMediaDecoder::GetDebugInfo().

::: dom/media/ChannelMediaDecoder.cpp:621
(Diff revision 2)
>  }
>  
>  nsCString
>  ChannelMediaDecoder::GetDebugInfo()
>  {
>    auto&& str = MediaDecoder::GetDebugInfo();

`MediaDecoder::GetDebugInfo()` returns `nsCString`.
I would suggest you explicitly declare `nsCString str` instead of the mysterious `auto&&`, so it's obvious the type of `str` matches what this function returns.

...

::: dom/media/ChannelMediaDecoder.cpp:625
(Diff revision 2)
>  {
>    auto&& str = MediaDecoder::GetDebugInfo();
>    if (mResource) {
>      AppendStringIfNotEmpty(str, mResource->GetDebugInfo());
>    }
> -  return str;
> +  return Move(str);

...

And therefore now we don't need this `Move`.

(Or is the clang warning still visible here? If yes, does it mention the differing types?)
Attachment #8971209 - Flags: review?(gsquelart) → review+
(In reply to Gerald Squelart [:gerald] from comment #6)
> (Or is the clang warning still visible here? If yes, does it mention the
> differing types?)

It's now the same type, so it behaves as expected. I've made this change.
Attachment #8971209 - Flags: review?(jyavenard)
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/dcbdde8d683c
Move instead of copying strings returned from various GetDebugInfo functions for HTMLMediaElement::MozDumpDebugInfo. r=gerald
Comment on attachment 8971209 [details]
Bug 1457137 - Move instead of copying strings returned from various GetDebugInfo functions for HTMLMediaElement::MozDumpDebugInfo.

https://reviewboard.mozilla.org/r/239994/#review246116

::: dom/media/MediaDecoderStateMachine.cpp:3784
(Diff revision 3)
>  MediaDecoderStateMachine::GetDebugInfo()
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    int64_t duration =
>      mDuration.Ref() ? mDuration.Ref().ref().ToMicroseconds() : -1;
>    auto str = nsPrintfCString(

nstead of using auto here, we could have used nsCString str =...
And then no need for move. Seems clearer to
Attachment #8971209 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/dcbdde8d683c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.