1433970 - AsmJSCache.cpp Parameter 'aWriteParams' is passed by value. It could be passed as a (const) reference which is usually faster and recommended in C++.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[jbonnafo]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180118215408

Steps to reproduce:

Static code analysis highlights that aWriteParams should be passed by (const) reference


Actual results:

In current code, parameter is passed by value


Expected results:

It could be passed as a (const) reference which is usually faster and recommended in C++.

Comment 1

[jbonnafo]

Attachment: Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference

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

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8946323 [details]
Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference

https://reviewboard.mozilla.org/r/216306/#review222062

Thanks.

::: commit-message-2cc71:1
(Diff revision 1)
> +Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference r?froydnj

Nit: there's usually no need to specify the file you're modifying in the commit message; it's self-evident from the patch.

I think I'd also say "pass WriteParams parameters by const reference", since that's talking about the specific values being passed, rather than the argument names, which could be anything.

Comment 3

[jbonnafo]

Thanks for your review and comment!

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thank you for the patch. You have to set the issue open in Review Board as fixed by commit so the patch can land.

Comment 5

[jbonnafo]

Comment on attachment 8946323 [details]
Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference

https://reviewboard.mozilla.org/r/216306/#review222088

::: commit-message-2cc71:1
(Diff revision 1)
> +Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference r?froydnj

Ok thanks

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to jbonnafo from comment #5)
> Comment on attachment 8946323 [details]
> Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference
> 
> https://reviewboard.mozilla.org/r/216306/#review222088
> 
> ::: commit-message-2cc71:1
> (Diff revision 1)
> > +Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference r?froydnj
> 
> Ok thanks

You need to address the issue by uploading a commit with a new commit message, not just by marking the issue as resolved.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> You need to address the issue by uploading a commit with a new commit
> message, not just by marking the issue as resolved.

To be clearer: upload a new commit with a fixed commit message, and then mark the issue as resolved.

Comment 8

[jbonnafo]

Comment on attachment 8946323 [details]
Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference

https://reviewboard.mozilla.org/r/216306/#review222822

::: commit-message-2cc71:1
(Diff revision 1)
> +Bug 1433970 - AsmJSCache.cpp pass aWriteParams by const reference r?froydnj

Hello,

I have amended the commit message accordingly.
I hope it is better now.

Thanks,

Comment 9

[jbonnafo]

Attachment: Bug 1433970 - pass WriteParams parameters by const reference

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

Comment 10

[jbonnafo]

I have tried a new proposal, i hope it is better now.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8947607 [details]
Bug 1433970 - pass WriteParams parameters by const reference

https://reviewboard.mozilla.org/r/217296/#review223302

Thank you!

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to jbonnafo from comment #10)
> I have tried a new proposal, i hope it is better now.

Yes, it's better, thanks.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/492fb9c24ba5
pass WriteParams parameters by const reference r=froydnj

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/492fb9c24ba5