982212 - r-value references for nsTArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

nsTArray should allow using r-value references.

Comment 1

[alexander :surkov (:asurkov)]

Kyle, can you estimate pls when this bug can be fixed?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Implement r-value support for constructors and operator=

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2 - Implement r-value support for AppendElement

I could do Insert/ReplaceElementAt too if we think it'll be useful.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 3: Remove workarounds for lack of r-value support added in bug 967364

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Unfortunately b2g doesn't like these patches.  I think it's a compiler issue with the ancient (4.4) version of GCC it is using :/

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8391787 [details] [diff] [review]
Part 1: Implement r-value support for constructors and operator=

Review of attachment 8391787 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/TestTArray.cpp
@@ +249,5 @@
> +class Countable {
> +  static uint32_t sCount;
> +
> +  public:
> +    Countable() 

Nit: trailing whitespace.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8391788 [details] [diff] [review]
Part 2 - Implement r-value support for AppendElement

Review of attachment 8391788 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsTArray.h
@@ +1275,4 @@
>    template<class Item>
> +  elem_type *AppendElement(Item&& item) {
> +    if (!Alloc::Successful(this->EnsureCapacity(Length() + 1, sizeof(elem_type))))
> +      return nullptr;

This duplication is all kind of unfortunate, but I don't see a better way to do this. :(

Comment 8

[alexander :surkov (:asurkov)]

is there anything left before the patches can be landed?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to alexander :surkov from comment #8)
> is there anything left before the patches can be landed?

Comment 5.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #9)
> (In reply to alexander :surkov from comment #8)
> > is there anything left before the patches can be landed?
> 
> Comment 5.

can you give any estimations, are there any nearest plans to fix that? Currently this bug blocks some a11y work. So I need to figure out if I should wait for the fix or workaround the issue.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> Unfortunately b2g doesn't like these patches.  I think it's a compiler issue
> with the ancient (4.4) version of GCC it is using :/

I'm guessing it isn't obvious how workaround the 4.4 error(s)? Unfortunately I don't see a bug filed about fixing the gcc 4.7+ issues with building b2g...

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, it's not.  Either somebody needs to figure out how to work around it or we need to upgrade the compiler.  I'm also about to go on vacation until mid-May ...

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

At least some of this was done in bug 1039521.

Comment 14

[Peter Van der Beken [:peterv]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> Unfortunately b2g doesn't like these patches.  I think it's a compiler issue
> with the ancient (4.4) version of GCC it is using :/

Was it failing to compile? I pushed updated patches to try (B2G ICS Emulator builds) and it looks green: https://tbpl.mozilla.org/?tree=Try&rev=d2ab0efb31d1.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: Part 1: Implement r-value support for constructors and operator=

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Part 2 - Implement r-value support for AppendElement

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: Part 3 - Now that nsTArray::AppendElement supports r-value references, remove the workarounds added in Bug 967364

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, the tests explode.

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: Part 1a: Implement r-value support for constructors and operator=

This somehow fixes the oranges with gcc 4.4. The problem is related to the copying of an array here: http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=db7212847c14#141 If I replace that with an empty nsTArray and then copy using AppendElements the orange goes away too. I haven't debugged much further, since I think this patch is correct and it fixes the orange.

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: Part 4 - Remove explicit from the nsTArray-constructor taking an r-value and add MoveElementsFrom taking an r-value

I'd also like to do this. I'm not completely sure, but I think removing explicit from that move constructor is fine, and it allows code like:

nsTArray<...> Foo()
{
  nsTArray<...> foo;
  ...
  return Move(foo);
}

nsTArray<...> bar(Foo());

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to Peter Van der Beken [:peterv] from comment #20)
> Created attachment 8484829 [details] [diff] [review]
> Part 4 - Remove explicit from the nsTArray-constructor taking an r-value and
> add MoveElementsFrom taking an r-value
> 
> I'd also like to do this. I'm not completely sure, but I think removing
> explicit from that move constructor is fine, and it allows code like:
> 
> nsTArray<...> Foo()
> {
>   nsTArray<...> foo;
>   ...
>   return Move(foo);
> }
> 
> nsTArray<...> bar(Foo());

Hm, this is actually worse than what we had before, isn't it?  Before, the compiler would have optimized things such that we were directly constructing |foo| in the return value, but now, we have to construct a temporary |foo| and then Move() it into the return value, which is extra work.

What does the compiler complain about if you have the |explicit|?

Comment 22

[Peter Van der Beken [:peterv]]

error: no matching constructor for initialization of 'nsTArray<nsString>'
    return mozilla::Move(foo);
           ^~~~~~~~~~~~~~~~~~
../../dist/include/nsTArray.h:1754:3: note: candidate constructor not viable: requires 0 arguments, but 1 was provided
  nsTArray() {}
  ^

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8484829 [details] [diff] [review]
Part 4 - Remove explicit from the nsTArray-constructor taking an r-value and add MoveElementsFrom taking an r-value

Review of attachment 8484829 [details] [diff] [review]:
-----------------------------------------------------------------

This is OK.  Bonus points for writing a test to verify that nsTArray's move constructor works.

::: xpcom/glue/nsTArray.h
@@ +1761,5 @@
>      : base_type(aOther)
>    {
>    }
>    template<class Allocator>
> +  nsTArray(nsTArray_Impl<E, Allocator>&& aOther)

If it's not going to be explicit, then it should be MOZ_IMPLICIT.

Comment 24

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #23)
> This is OK.  Bonus points for writing a test to verify that nsTArray's move
> constructor works.

Oh, never mind, an earlier patch includes those.  Great!

Comment 25

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b53e41fe235
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1c0fde4eee2
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b0a09414a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b01f733eeac
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0b468fb407

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b53e41fe235
https://hg.mozilla.org/mozilla-central/rev/d1c0fde4eee2
https://hg.mozilla.org/mozilla-central/rev/88b0a09414a4
https://hg.mozilla.org/mozilla-central/rev/6b01f733eeac
https://hg.mozilla.org/mozilla-central/rev/ad0b468fb407