struct MyStruct;

struct Bar {
  Bar();
  ~Bar();
  nsTArray<MyStruct> mArray;
};

Should probably work.

If you really only have a forward declaration of MyStruct, you need to declare whether MyStruct is copy-constructible by either MOZ_DECLARE_COPY_CONSTRUCTIBLE or MOZ_DECLARE_NON_COPY_CONSTRUCTIBLE.

Why? This works with std::vector and so on.

Can we move the template magic from CopyEnabler to the copy-constructor so that you only need to evaluate IsCopyConstructible if you try to call it?

Though I guess std::vector is copy-constructible even if T isn't... I don't understand the reasoning for https://phabricator.services.mozilla.com/D66244, can you elaborate? This will cause people to just add a bunch of otherwise-useless includes...

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Why? This works with std::vector and so on.

Yes, it works with std::vector in libstdc++ and libc++. Not sure if this is required by the standard (maybe yes), and what exactly "and so on" includes, but probably it works with a number of other containers as well. But does that imply there shouldn't be any containers that rely on a complete definition of its argument types? (which definitely isn't true for templates in general other than containers)

Can we move the template magic from CopyEnabler to the copy-constructor so that you only need to evaluate IsCopyConstructible if you try to call it?

I am not completely sure what you suggest here. Can you make this more specific? Adding any user-provided copy constructor to nsTArray_Impl would defeat the purpose of the CopyEnabler.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Though I guess std::vector is copy-constructible even if T isn't...

I tried it and you're right insofar static_assert(std::is_copy_constructible_v<std::vector<std::unique_ptr<int>>>); holds, but of course the copy constructor cannot be instantiated either.

I don't understand the reasoning for https://phabricator.services.mozilla.com/D66244, can you elaborate?

I don't remember exactly, but it was somehow related to gcc choosing the copy constructor or assignment operator, which couldn't be instantiated. Try to revert https://phabricator.services.mozilla.com/D66244 and https://phabricator.services.mozilla.com/D66247 to see what happens when compiling with gcc then. If you don't want to do that, I can check it again. I am not completely sure why clang doesn't bail out as well.

I am not sure if this is only an outcome of the existing of FallibleTArray (which ought to be removed eventually) and/or AutoTArray deriving from nsTArray and/or many of our types missing to specify noexcept on their move operations. Just speculating.

In general, I think it's not good if a type claims std::is_copy_constructible but actually it isn't. It seems to defeat the purpose of these type traits and renders them useless.

This will cause people to just add a bunch of otherwise-useless includes...

I am not sure what you mean by "useless" here. The Google coding style, which we adopt, discourages the use of forward declarations: https://google.github.io/styleguide/cppguide.html#Forward_Declarations

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Why? This works with std::vector and so on.

Yes, it works with std::vector in libstdc++ and libc++. Not sure if this is required by the standard (maybe yes), and what exactly "and so on" includes, but probably it works with a number of other containers as well. But does that imply there shouldn't be any containers that rely on a complete definition of its argument types? (which definitely isn't true for templates in general other than containers)

Can we move the template magic from CopyEnabler to the copy-constructor so that you only need to evaluate IsCopyConstructible if you try to call it?

I am not completely sure what you suggest here. Can you make this more specific? Adding any user-provided copy constructor to nsTArray_Impl would defeat the purpose of the CopyEnabler.

I meant something like:

template <typename = typename std::enable_if_t<std::is_copy_constructible_v<E>>>
nsTArray(const nsTArray&);

or something. But I couldn't make something like that work.

This will cause people to just add a bunch of otherwise-useless includes...

I am not sure what you mean by "useless" here. The Google coding style, which we adopt, discourages the use of forward declarations: https://google.github.io/styleguide/cppguide.html#Forward_Declarations

I don't read "Try to avoid forward declarations of entities defined in another project" as "discourages the use of forward declarations"... Forward declarations are one of the best ways to avoid blowing up compile times.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Why? This works with std::vector and so on.

Yes, it works with std::vector in libstdc++ and libc++. Not sure if this is required by the standard (maybe yes), and what exactly "and so on" includes, but probably it works with a number of other containers as well. But does that imply there shouldn't be any containers that rely on a complete definition of its argument types? (which definitely isn't true for templates in general other than containers)

Can we move the template magic from CopyEnabler to the copy-constructor so that you only need to evaluate IsCopyConstructible if you try to call it?

I am not completely sure what you suggest here. Can you make this more specific? Adding any user-provided copy constructor to nsTArray_Impl would defeat the purpose of the CopyEnabler.

I meant something like:

template <typename = typename std::enable_if_t<std::is_copy_constructible_v<E>>>
nsTArray(const nsTArray&);

or something. But I couldn't make something like that work.

Ok, then it's what I thought. Yes, that doesn't work. You can't provide a copy constructor conditionally relying on SFINAE, this is the very reason the CopyEnabler exists. What could be done differently in theory were not derive from a class template depending on the type trait but having this on a data member instead, but I am not sure if this would change anything about requiring a complete definition of the type.

This will cause people to just add a bunch of otherwise-useless includes...

I am not sure what you mean by "useless" here. The Google coding style, which we adopt, discourages the use of forward declarations: https://google.github.io/styleguide/cppguide.html#Forward_Declarations

I don't read "Try to avoid forward declarations of entities defined in another project" as "discourages the use of forward declarations"... Forward declarations are one of the best ways to avoid blowing up compile times.

Well, the wording is not entirely clear. The first sentence under the heading has no restriction to "another project" (whatever that means) and the two bullet points after the one you cited don't have it either. It may have a significant influence on compile-times in the case it is a widely included header and its users don't require the definitions of the involved types anyway. Personally, I would still try to address this in some way other than with forward declarations, but this is somewhat out of scope here.

Getting back to the scope of the bug: is using the macros I mentioned or providing a definition of the type acceptable here?

Getting back to the scope of the bug: is using the macros I mentioned or providing a definition of the type acceptable here?

Well, not sure... I sure could make it work like that, or I can add the #include. If I use the macro then there's two pieces of code that I need to keep track of. In this case it was an ipdl-generated type. If the code generation changes, then the macro usage can get out of sync.

Plus in this case I don't even care if the array is copy-constructible or not, I just have a single array where I emplace elements and move away at some point.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Getting back to the scope of the bug: is using the macros I mentioned or providing a definition of the type acceptable here?

Well, not sure... I sure could make it work like that, or I can add the #include. If I use the macro then there's two pieces of code that I need to keep track of. In this case it was an ipdl-generated type. If the code generation changes, then the macro usage can get out of sync.

Can you share the patch with me?

Plus in this case I don't even care if the array is copy-constructible or not, I just have a single array where I emplace elements and move away at some point.

You could just use MOZ_DECLARE_NON_COPY_CONSTRUCTIBLE then and get a non-copyable nsTArray even if the element type is copyable.
(And if you wrongfully used MOZ_DECLARE_COPY_CONSTRUCTIBLE, no bad things happen. I am not completely sure, it either will fail to compile at all or you get back to the state before D66244, with the nsTArray claiming is_copy_constructible, but copy operations that cannot be instantiated).

Another option might be to use mozilla/Vector.h?

After doing some more work on nsTArray, I think that nsTArray should be made uncopyable in general. Copies may be expensive, so doing this accidentally/implicitly doesn't seem a good idea. We can add CopyableTArray for the rare cases where this is really required, and no named member functions can be used explicitly instead. See also Bug 1628692, which is about FallibleTArray, where this is even more problematic.

For the regular nsTArray, this will allow the use with incomplete types again. Also, MOZ_DECLARE_NON_COPY_CONSTRUCTIBLE and MOZ_DECLARE_COPY_CONSTRUCTIBLE can be removed again.

Depends on D72175

Depends on D72315

Depends on D72316

Depends on D72317

Depends on D72318

Depends on D72319

Depends on D72320

Depends on D72321

Depends on D72322

Depends on D72323

Depends on D72325

Depends on D72326

Depends on D72327

Depends on D72328

Depends on D72330

Depends on D72331

Depends on D72332

Depends on D72341

Depends on D72343

Depends on D72344

Depends on D72345

Depends on D72346

Depends on D72347

Depends on D72349

Depends on D72350

Depends on D72351

Depends on D72352

Depends on D72353

Depends on D72354

Depends on D72355

Depends on D72315

https://hg.mozilla.org/mozilla-central/rev/e8d629c316f9
https://hg.mozilla.org/mozilla-central/rev/0ca000a364b4
https://hg.mozilla.org/mozilla-central/rev/078ec53cf0f5
https://hg.mozilla.org/mozilla-central/rev/646f434f456d
https://hg.mozilla.org/mozilla-central/rev/2505fccc09c9
https://hg.mozilla.org/mozilla-central/rev/fdef6c89e60f
https://hg.mozilla.org/mozilla-central/rev/ef52aacd0d0c
https://hg.mozilla.org/mozilla-central/rev/edb8086fa7b9
https://hg.mozilla.org/mozilla-central/rev/a883227e3bf3
https://hg.mozilla.org/mozilla-central/rev/0648d1512c8e
https://hg.mozilla.org/mozilla-central/rev/0dabfb14c68a
https://hg.mozilla.org/mozilla-central/rev/e1e24ef3b06c
https://hg.mozilla.org/mozilla-central/rev/79aa34a3c0e2
https://hg.mozilla.org/mozilla-central/rev/ecd8d7595c98
https://hg.mozilla.org/mozilla-central/rev/1d6e1743691e
https://hg.mozilla.org/mozilla-central/rev/b0395e5dc5e6

Depends on D72356

Depends on D73624

Depends on D73625

Depends on D73626

Depends on D73627

Depends on D73628

Depends on D73629

Depends on D73630

Depends on D73631

Depends on D73633

Depends on D73635

Depends on D73637

Depends on D73638

Depends on D73639

Depends on D73640

Depends on D73641

Depends on D73642

Depends on D73643

Depends on D73644

Depends on D73645

Depends on D73646

Depends on D73647

Depends on D73649

Depends on D73650

Depends on D73651

Depends on D73652

Depends on D73653

Depends on D73654

Depends on D73655

Depends on D73656

Depends on D73657

Depends on D73658

Depends on D73659

Depends on D73660

Depends on D73661

Depends on D73663

Depends on D73664

Depends on D73665

Depends on D73666

Depends on D73667

Depends on D73668

Depends on D73669

Depends on D73670

Depends on D73671

Depends on D73672

Depends on D73673

Depends on D73674

Depends on D73676

Depends on D73677

Depends on D73678

Depends on D73679

Depends on D73681

Depends on D73682

Depends on D73683

Depends on D73685

Depends on D73683

Depends on D73817

Backed out for build bustages.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=a3f17d39223414d7ed0af8095e1b4d810c8dce0d&selectedTaskRun=bJs_XUnqRVCPCKc2ckVRHg-0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=300842605&repo=autoland&lineNumber=38531

Backout: https://hg.mozilla.org/integration/autoland/rev/d743835c5aa45291b21ff12dcd7566393869164f

D73642 must probably land together this those patches. I am checking this again, and will then re-land.

https://hg.mozilla.org/mozilla-central/rev/00793c67119e
https://hg.mozilla.org/mozilla-central/rev/b698a70baad9
https://hg.mozilla.org/mozilla-central/rev/6a15c418a163
https://hg.mozilla.org/mozilla-central/rev/ebc95a347a4d
https://hg.mozilla.org/mozilla-central/rev/a4394254942f
https://hg.mozilla.org/mozilla-central/rev/b82c21f49cf0
https://hg.mozilla.org/mozilla-central/rev/cb92328d2f98
https://hg.mozilla.org/mozilla-central/rev/ea2ebdfc8420
https://hg.mozilla.org/mozilla-central/rev/fcb1c15017d7
https://hg.mozilla.org/mozilla-central/rev/209f82a9305c
https://hg.mozilla.org/mozilla-central/rev/0a74c3e1493e
https://hg.mozilla.org/mozilla-central/rev/bc91cd947dce
https://hg.mozilla.org/mozilla-central/rev/a2a27f276532
https://hg.mozilla.org/mozilla-central/rev/81b877a1173d
https://hg.mozilla.org/mozilla-central/rev/845efef1d2f6
https://hg.mozilla.org/mozilla-central/rev/525842a74603
https://hg.mozilla.org/mozilla-central/rev/51b22311b4d3
https://hg.mozilla.org/mozilla-central/rev/1af19ba39456
https://hg.mozilla.org/mozilla-central/rev/0cb3cee0d145
https://hg.mozilla.org/mozilla-central/rev/10abd1ef75fb
https://hg.mozilla.org/mozilla-central/rev/7a510d21948f
https://hg.mozilla.org/mozilla-central/rev/ae0a12a3935f
https://hg.mozilla.org/mozilla-central/rev/704cedb103b8
https://hg.mozilla.org/mozilla-central/rev/078ce9d263f9
https://hg.mozilla.org/mozilla-central/rev/b045baf40b3b
https://hg.mozilla.org/mozilla-central/rev/2136c5964756
https://hg.mozilla.org/mozilla-central/rev/68cc4148bda2
https://hg.mozilla.org/mozilla-central/rev/44523b655777
https://hg.mozilla.org/mozilla-central/rev/442990d2263c
https://hg.mozilla.org/mozilla-central/rev/a83bdead5cf4
https://hg.mozilla.org/mozilla-central/rev/beb39c53935c
https://hg.mozilla.org/mozilla-central/rev/2440385dd6ed
https://hg.mozilla.org/mozilla-central/rev/01d9c38b8b92
https://hg.mozilla.org/mozilla-central/rev/2b8cca4379a3
https://hg.mozilla.org/mozilla-central/rev/400ef1fdd137
https://hg.mozilla.org/mozilla-central/rev/515a0edacecf
https://hg.mozilla.org/mozilla-central/rev/3b4806b00aec
https://hg.mozilla.org/mozilla-central/rev/4ff37b98727e

https://hg.mozilla.org/mozilla-central/rev/21d9662b7139
https://hg.mozilla.org/mozilla-central/rev/8e28d10177c0
https://hg.mozilla.org/mozilla-central/rev/e8360d8d9140
https://hg.mozilla.org/mozilla-central/rev/90087b975479
https://hg.mozilla.org/mozilla-central/rev/340387460cea
https://hg.mozilla.org/mozilla-central/rev/1cf33b46f79c
https://hg.mozilla.org/mozilla-central/rev/0c3511b04a08
https://hg.mozilla.org/mozilla-central/rev/f632c3de9167
https://hg.mozilla.org/mozilla-central/rev/047eb92da0b8
https://hg.mozilla.org/mozilla-central/rev/f876865a0809
https://hg.mozilla.org/mozilla-central/rev/bdf50b0f550e
https://hg.mozilla.org/mozilla-central/rev/242b4a61389a
https://hg.mozilla.org/mozilla-central/rev/b2957781f934
https://hg.mozilla.org/mozilla-central/rev/710da9ba5df9
https://hg.mozilla.org/mozilla-central/rev/db5a127f11e4
https://hg.mozilla.org/mozilla-central/rev/aa89500e0794
https://hg.mozilla.org/mozilla-central/rev/8b451c877b10
https://hg.mozilla.org/mozilla-central/rev/49bbb28670a3
https://hg.mozilla.org/mozilla-central/rev/69f7e3e32072
https://hg.mozilla.org/mozilla-central/rev/186eac923253

https://hg.mozilla.org/mozilla-central/rev/2b205fe4f496
https://hg.mozilla.org/mozilla-central/rev/6b2268b85ba9
https://hg.mozilla.org/mozilla-central/rev/6ff464bd2190
https://hg.mozilla.org/mozilla-central/rev/b60235fc5d62
https://hg.mozilla.org/mozilla-central/rev/c9cecffc0718
https://hg.mozilla.org/mozilla-central/rev/5d64fb7cb091

https://hg.mozilla.org/mozilla-central/rev/9b53d010f336
https://hg.mozilla.org/mozilla-central/rev/7b02e379d198
https://hg.mozilla.org/mozilla-central/rev/66d730089275
https://hg.mozilla.org/mozilla-central/rev/d80ba93e17be
https://hg.mozilla.org/mozilla-central/rev/8e8a16e4e40d
https://hg.mozilla.org/mozilla-central/rev/93b096e2d3da
https://hg.mozilla.org/mozilla-central/rev/b65169d47a91
https://hg.mozilla.org/mozilla-central/rev/7b7db00669fe

These patches seem to have had a noticeable effect on webaudio benchmarks (I'm not sure why perfherder didn't flag it.) https://treeherder.mozilla.org/perf.html#/graphs?highlightAlerts=1&highlightedRevisions=3736193c7268&highlightedRevisions=96ac9e3b85a6&selected=1922706,1129472227&series=autoland,1921254,1,10&series=autoland,1922706,1,10&timerange=5184000&zoom=1588377620670,1588903347294,100.40490196078431,159.82647058823528 Was this expected?

You probably refer to this step specifically? https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&newProject=autoland&originalRevision=d2bb25cf6ed55b09585f40f102d514f679469c33&newRevision=2136c5964756e8a9fc04d755986e907588588385&originalSignature=1921248&newSignature=1921248&framework=10#table-row-1197107037

If anything, improvements were expected where arrays are now moved which were copied before. What is conceivable is that copy elision might have been in effect before? The results are very mixed, some of the sub-benchmarks have improved, others have worsened. Can you point me to instructions to run these locally, so I can run them under a profiler and get a better understanding of what's happening?

The results are very mixed, some of the sub-benchmarks have improved, others have worsened.

The subtest viewer is not reliable when comparing consecutive autoland pushes, because there's only one data point from each, and these tests have noise. A potential way around this is to retrigger the wa jobs on the pushes until the confidence values go up. Alternatively you can look at the graphs to see history over many pushes, it's often clear when there's a jump.

Can you point me to instructions to run these locally, so I can run them under a profiler and get a better understanding of what's happening?

These tests can be run by navigating to third_party/webkit/PerformanceTests/webaudio/index.html?raptor although you may need to serve that from local HTTP if it doesn't work over the file: protocol.