AutoTArray Copy Constructor forgets to explicitly call a parent constructor

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
This causes a compiler warning on Linux when someone actually tries to use it.

error: base class 'class nsTArray<Strip>' should be explicitly initialized in the copy constructor [-Werror=extra]

Comment 2

a year ago
mozreview-review
Comment on attachment 8957312 [details]
Bug 1444200: Explicitly call the parent class default constructor from AutoTArray's copy constructor.

https://reviewboard.mozilla.org/r/226212/#review232140

I think this works, but:

1. am a little curious about the performance implications--we don't *need* to explicitly call the parent class default constructor, because we're just going to overwrite the header it writes.  I don't know whether the compiler is smart enough to optimize away the dead write; and
2. why would you need to copy one of these?
Attachment #8957312 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 3

a year ago
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8957312 [details]
> Bug 1444200: Explicitly call the parent class default constructor from
> AutoTArray's copy constructor.
> 
> https://reviewboard.mozilla.org/r/226212/#review232140
> 
> I think this works, but:
> 
> 1. am a little curious about the performance implications--we don't *need*
> to explicitly call the parent class default constructor, because we're just
> going to overwrite the header it writes.  I don't know whether the compiler
> is smart enough to optimize away the dead write; and

The default constructor -always- gets called I believe. It just normally gets called implicitly, that's what the warning is about :).

> 2. why would you need to copy one of these?

It's a member of a class that I copy in a patch. I could use nsTArray, but AutoTArray makes a good 30-40% perf difference for my particular usecase.
(Assignee)

Comment 4

a year ago
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8957312 [details]
> Bug 1444200: Explicitly call the parent class default constructor from
> AutoTArray's copy constructor.
> 
> https://reviewboard.mozilla.org/r/226212/#review232140
> 
> I think this works, but:
> 
> 1. am a little curious about the performance implications--we don't *need*
> to explicitly call the parent class default constructor, because we're just
> going to overwrite the header it writes.  I don't know whether the compiler
> is smart enough to optimize away the dead write; and
> 2. why would you need to copy one of these?

Having said that, if we wanted to improve performance, we could add an explicit, protected constructor on the -parent- class that avoids the write, which would be the solution I believe we'd need if we're worried the compiler doesn't optimize it away (since it's all inlined, it might, I'm not sure).
Flags: needinfo?(nfroyd)

Comment 5

a year ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e333746d633
Explicitly call the parent class default constructor from AutoTArray's copy constructor. r=froydnj
(In reply to Bas Schouten (:bas.schouten) from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > Comment on attachment 8957312 [details]
> > Bug 1444200: Explicitly call the parent class default constructor from
> > AutoTArray's copy constructor.
> > 
> > https://reviewboard.mozilla.org/r/226212/#review232140
> > 
> > I think this works, but:
> > 
> > 1. am a little curious about the performance implications--we don't *need*
> > to explicitly call the parent class default constructor, because we're just
> > going to overwrite the header it writes.  I don't know whether the compiler
> > is smart enough to optimize away the dead write; and
> > 2. why would you need to copy one of these?
> 
> Having said that, if we wanted to improve performance, we could add an
> explicit, protected constructor on the -parent- class that avoids the write,
> which would be the solution I believe we'd need if we're worried the
> compiler doesn't optimize it away (since it's all inlined, it might, I'm not
> sure).

I'll check whether the write actually gets optimized away, and file a bug if we need to do that.
Flags: needinfo?(nfroyd)

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3e333746d633
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.