1329243 - Fix nsAdoptingString -Wextra copy constructor warning

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[:Cykesiopka]

Trying to enable -Wextra for GCC for the security/certverifier/ folder results
in this build error if --enable-warnings-as-errors is on:
> nsTString.h: In copy constructor ‘nsAdoptingString::nsAdoptingString(const self_type&)’:
> string-template-def-unichar.h:23:45: error: base class ‘class nsXPIDLString’ should be explicitly initialized in the copy constructor [-Werror=extra]

It would be nice to fix this so -Wextra can be used.

Comment 1

[Vincent Lequertier]

Hello, can I work on this ? :-)

Comment 2

[:Cykesiopka]

(In reply to vi.le from comment #1)
> Hello, can I work on this ? :-)
Sure. I'm not working on it at least.

Commit history for nsTString.h suggests you want to try asking :froydnj for review.

Thanks!

Comment 3

[Vincent Lequertier]

I'm able to reproduce the issue

Comment 4

[Vincent Lequertier]

Attachment: Bug 1329243 - Fix nsAdoptingString -Wextra copy constructor warning;

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

Comment 5

[Vincent Lequertier]

Should I add the '-Wextra' flag for the folder in the same patch?

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8828067 [details]
Bug 1329243 - Fix nsAdoptingString -Wextra copy constructor warning;

https://reviewboard.mozilla.org/r/105584/#review106790

This looks good, but it needs one modification before it's ready to land.  Please make the constructor call `nsTXPIDLString_CharT()` instead and re-submit the patch for review.  Thanks!

::: xpcom/string/nsTString.h:859
(Diff revision 1)
> +    : nsTXPIDLString_CharT(aStr)
>    {
>      *this = aStr;

I think it would be preferable to call the no-arg constructor for `nsTXPIDLString_CharT` here.  Otherwise, we'll have the `nsTXPIDLString_Char(const self_type&)` constructor calling `Assign`, and then we'll have the actual body of the `nsTAdoptingString_CharT` constructor calling `operator=`.  `Assign` will probably do a non-trivial amount of work, and we're just going to overwrite all the members of the string with the `operator=` call.

Comment 7

[Vincent Lequertier]

Attachment: use the no-arg constructor

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

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8829563 [details]
use the no-arg constructor

https://reviewboard.mozilla.org/r/106620/#review107928

Thank you!

Comment 9

[Vincent Lequertier]

Do I need to squash my commits?

Comment 10

[:Cykesiopka]

(In reply to vi.le from comment #9)
> Do I need to squash my commits?

Yes - please use the changes of commit 2, but with the commit message of commit 1.

In case you haven't already, please push to the try server as well:
https://wiki.mozilla.org/Build:TryServer
(I'm not sure if there are any tests that directly test nsAdoptingString, but a try push that at least proves that everything builds on (Android, Linux, OSX, Windows) would be good.)

Thanks!

Comment 11

[Vincent Lequertier]

Comment on attachment 8828067 [details]
Bug 1329243 - Fix nsAdoptingString -Wextra copy constructor warning;

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

Comment 12

[Vincent Lequertier]

I have requested a level 1 commit access here: https://bugzilla.mozilla.org/show_bug.cgi?id=1335229

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8828067 [details]
Bug 1329243 - Fix nsAdoptingString -Wextra copy constructor warning;

https://reviewboard.mozilla.org/r/105584/#review109616

Comment 14

[Vincent Lequertier]

Cykesiopka, can you push to the try server for me please ?

Comment 15

[:Cykesiopka]

(In reply to vi.le from comment #14)
> Cykesiopka, can you push to the try server for me please ?

Looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f92536791e5fc5ea922e0b9df56a3838441da7d5

Again, thanks for fixing this.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8878f903df2
Fix nsAdoptingString -Wextra copy constructor warning; r=froydnj

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/a8878f903df2