1444200 - AutoTArray Copy Constructor forgets to explicitly call a parent constructor

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Bas Schouten (:bas.schouten)]

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 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1444200: Explicitly call the parent class default constructor from AutoTArray's copy constructor.

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

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Bas Schouten (:bas.schouten)]

(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.

Comment 4

[Bas Schouten (:bas.schouten)]

(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).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e333746d633
Explicitly call the parent class default constructor from AutoTArray's copy constructor. r=froydnj

Comment 6

[Nathan Froyd [: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.

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3e333746d633