1317430 - Avoid a unnecessary copy by passing a const &

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1317430 - Avoid a unnecessary copy by passing a const & in dom/base/

Review commit: https://reviewboard.mozilla.org/r/92794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92794/

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1317430 - Avoid a unnecessary copy by passing a const & in dom/media/

Review commit: https://reviewboard.mozilla.org/r/92868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92868/

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1317430 - Avoid a unnecessary copy by passing a const & in extensions/spellcheck/

Review commit: https://reviewboard.mozilla.org/r/92870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92870/

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1317430 - Avoid a unnecessary copy by passing a const & in gfx/

Review commit: https://reviewboard.mozilla.org/r/92872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92872/

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1317430 - Avoid a unnecessary copy by passing a const & in js/

Review commit: https://reviewboard.mozilla.org/r/92874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92874/

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1317430 - Avoid a unnecessary copy by passing a const & in layout/

Review commit: https://reviewboard.mozilla.org/r/92876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92876/

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1317430 - Avoid a unnecessary copy by passing a const & in netwerk/

Review commit: https://reviewboard.mozilla.org/r/92878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92878/

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1317430 - Avoid a unnecessary copy by passing a const & in view/

Review commit: https://reviewboard.mozilla.org/r/92880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92880/

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8810591 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in view/

https://reviewboard.mozilla.org/r/92880/#review93010

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8810586 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in extensions/spellcheck/

https://reviewboard.mozilla.org/r/92870/#review93012

Comment 11

[Nicolas Silva [:nical]]

Comment on attachment 8810587 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in gfx/

https://reviewboard.mozilla.org/r/92872/#review93038

Does this help with reducing the static analysis / warnings noise? while it's preferable to pass objects with non-trivial destructors (like sk_sp) by const ref (or move) rather than value when possible, I don't think that passing Maybe<uint32_t> by reference brings any improvement (I am certain it won't make things noticeably worse either).

Comment 12

[Andi [:andi]]

Comment on attachment 8810587 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in gfx/

https://reviewboard.mozilla.org/r/92872/#review93038

This could help when passing large objects by value, that need to be copied on the stack of the calling function. There are some static analysis checkers that detect this and assert it as performance inneficient.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8810584 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in dom/base/

https://reviewboard.mozilla.org/r/92794/#review93058

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8810585 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in dom/media/

https://reviewboard.mozilla.org/r/92868/#review93052

::: dom/media/MediaFormatReader.h:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #if !defined(MediaFormatReader_h_)
>  #define MediaFormatReader_h_
>  
> +#include <utility>

Please don't use std::move, use mfbt Move instead

::: dom/media/MediaFormatReader.h:574
(Diff revision 1)
>    void OnVideoSeekFailed(const MediaResult& aError);
>    bool mSeekScheduled;
>  
>    void NotifyCompositorUpdated(RefPtr<layers::KnowsCompositor> aKnowsCompositor)
>    {
> -    mKnowsCompositor = aKnowsCompositor;
> +    mKnowsCompositor = std::move(aKnowsCompositor);

aKnowsCompositor.forget() if you want remove the unecessary refcount increase

::: dom/media/MediaSegment.h:142
(Diff revision 1)
>    PrincipalHandle GetLastPrincipalHandle() const { return mLastPrincipalHandle; }
>    /**
>     * Called by the MediaStreamGraph as it appends a chunk with a different
>     * principal id than the current one.
>     */
> -  void SetLastPrincipalHandle(PrincipalHandle aLastPrincipalHandle)
> +  void SetLastPrincipalHandle(const PrincipalHandle& aLastPrincipalHandle)

I am not familiar with MediaSegment and PrincipalHandle

I do not know how SetLastPrincipalHandle is called and if the passing by value wasn't intentional (it appears not but just in case..

Being a RefPtr, there's no much advantage being using a reference of the pointer by value.

I let that being reviewed by its author.

::: dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp:37
(Diff revision 1)
>  }
>  
>  void
>  WidevineDecryptor::SetCDM(RefPtr<CDMWrapper> aCDM)
>  {
> -  mCDM = aCDM;
> +  mCDM = std::move(aCDM);

same as earlier.
Use Move(), or better, aCDM.forget()

Comment 15

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8810585 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in dom/media/

https://reviewboard.mozilla.org/r/92868/#review93062

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8810590 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in netwerk/

https://reviewboard.mozilla.org/r/92878/#review93068

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8810589 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in layout/

https://reviewboard.mozilla.org/r/92876/#review93116

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810586 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in extensions/spellcheck/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92870/diff/1-2/

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810587 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in gfx/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92872/diff/1-2/

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810588 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in js/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92874/diff/1-2/

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810589 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in layout/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92876/diff/1-2/

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810590 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in netwerk/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92878/diff/1-2/

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810591 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in view/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92880/diff/1-2/

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810585 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in dom/media/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92868/diff/1-2/

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 8810588 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in js/

https://reviewboard.mozilla.org/r/92874/#review93186

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810584 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in dom/base/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92794/diff/1-2/

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810586 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in extensions/spellcheck/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92870/diff/2-3/

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810587 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in gfx/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92872/diff/2-3/

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810588 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in js/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92874/diff/2-3/

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810590 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in netwerk/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92878/diff/2-3/

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810591 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in view/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92880/diff/2-3/

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810585 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in dom/media/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92868/diff/2-3/

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8810589 [details]
Bug 1317430 - Avoid a unnecessary copy by passing a const & in layout/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92876/diff/2-3/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2eaddb9f053a
Avoid a unnecessary copy by passing a const & in dom/base/ r=smaug
https://hg.mozilla.org/integration/autoland/rev/0d8fa19c7fa9
Avoid a unnecessary copy by passing a const & in extensions/spellcheck/ r=njn
https://hg.mozilla.org/integration/autoland/rev/90f23af9f67b
Avoid a unnecessary copy by passing a const & in gfx/ r=nical
https://hg.mozilla.org/integration/autoland/rev/3e2bacaba546
Avoid a unnecessary copy by passing a const & in js/ r=Waldo
https://hg.mozilla.org/integration/autoland/rev/8991ec245323
Avoid a unnecessary copy by passing a const & in netwerk/ r=valentin
https://hg.mozilla.org/integration/autoland/rev/c6c5eee7d566
Avoid a unnecessary copy by passing a const & in view/ r=njn
https://hg.mozilla.org/integration/autoland/rev/27b29b39a393
Avoid a unnecessary copy by passing a const & in dom/media/ r=jya,pehrsons
https://hg.mozilla.org/integration/autoland/rev/3c5b4130f199
Avoid a unnecessary copy by passing a const & in layout/ r=mstange

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2eaddb9f053a
https://hg.mozilla.org/mozilla-central/rev/0d8fa19c7fa9
https://hg.mozilla.org/mozilla-central/rev/90f23af9f67b
https://hg.mozilla.org/mozilla-central/rev/3e2bacaba546
https://hg.mozilla.org/mozilla-central/rev/8991ec245323
https://hg.mozilla.org/mozilla-central/rev/c6c5eee7d566
https://hg.mozilla.org/mozilla-central/rev/27b29b39a393
https://hg.mozilla.org/mozilla-central/rev/3c5b4130f199