Closed Bug 1329243 Opened 5 years ago Closed 4 years ago

Fix nsAdoptingString -Wextra copy constructor warning

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: Cykesiopka, Assigned: vi.le)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
Hello, can I work on this ? :-)
(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!
Assignee: nobody → vi.le
I'm able to reproduce the issue
Should I add the '-Wextra' flag for the folder in the same patch?
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.
Attachment #8828067 - Flags: review?(nfroyd)
Attachment #8829563 - Flags: review?(nfroyd)
Comment on attachment 8829563 [details]
use the no-arg constructor

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

Thank you!
Attachment #8829563 - Flags: review?(nfroyd) → review+
Do I need to squash my commits?
(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!
Status: NEW → ASSIGNED
Attachment #8829563 - Attachment is obsolete: true
I have requested a level 1 commit access here: https://bugzilla.mozilla.org/show_bug.cgi?id=1335229
Comment on attachment 8828067 [details]
Bug 1329243 - Fix nsAdoptingString -Wextra copy constructor warning;

https://reviewboard.mozilla.org/r/105584/#review109616
Attachment #8828067 - Flags: review?(nfroyd) → review+
Cykesiopka, can you push to the try server for me please ?
(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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a8878f903df2
Fix nsAdoptingString -Wextra copy constructor warning; r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8878f903df2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.