Closed Bug 982212 Opened 6 years ago Closed 5 years ago

r-value references for nsTArray

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(5 files, 3 obsolete files)

12.68 KB, patch
peterv
: review+
Details | Diff | Splinter Review
3.42 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.02 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.31 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.89 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
nsTArray should allow using r-value references.
Kyle, can you estimate pls when this bug can be fixed?
I could do Insert/ReplaceElementAt too if we think it'll be useful.
Attachment #8391788 - Flags: review?(benjamin)
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 :/
Attachment #8391787 - Flags: review?(benjamin) → review?(nfroyd)
Attachment #8391788 - Flags: review?(benjamin) → review?(nfroyd)
Attachment #8391789 - Flags: review?(benjamin) → review?(nfroyd)
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.
Attachment #8391787 - Flags: review?(nfroyd) → review+
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. :(
Attachment #8391788 - Flags: review?(nfroyd) → review+
Attachment #8391789 - Flags: review?(nfroyd) → review+
is there anything left before the patches can be landed?
(In reply to alexander :surkov from comment #8)
> is there anything left before the patches can be landed?

Comment 5.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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...
Flags: needinfo?(khuey)
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 ...
Flags: needinfo?(khuey)
At least some of this was done in bug 1039521.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(khuey)
No, the tests explode.
Flags: needinfo?(khuey)
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.
Attachment #8484827 - Flags: review?(nfroyd)
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());
Attachment #8484829 - Flags: review?(nfroyd)
Attachment #8484827 - Flags: review?(nfroyd) → review+
(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|?
Flags: needinfo?(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() {}
  ^
Flags: needinfo?(peterv)
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.
Attachment #8484829 - Flags: review?(nfroyd) → review+
(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!
Depends on: 1156538
You need to log in before you can comment on or make changes to this bug.