Bug 176432 – Vector should be able to easily create from a list of movable only items

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Vector should be able to easily create from a list of movable only items

[youenn fablet]

Created attachment 319953 [details]
Patch

[Daniel Bates]

Comment on attachment 319953 [details]
Patch

The code change  looks good. We just need to add unit tests.

[Darin Adler]

Comment on attachment 319953 [details]
Patch

Why does this have to be a named function rather than a constructor?

[Darin Adler]

Comment on attachment 319953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319953&action=review

> Source/WTF/wtf/Vector.h:632
> +        result.asanSetInitialBufferSizeTo(size);

I don’t understand why this is needed. The reserveInitialCapacity call should take care of this.

> Source/WTF/wtf/Vector.h:810
> +        uncheckedAppend(std::forward<U>(item));
> +        uncheckedAppend(std::forward<Items>(items)...);

Seems like we miss an opportunity here to optimize so we bump the size only once. This is going to increment the size over and over again, I think. Need to look at the generated code I guess.

[youenn fablet]

Created attachment 320037 [details]
Patch

[youenn fablet]

Thanks for the review.

(In reply to Daniel Bates from comment #2)
> Comment on attachment 319953 [details]
> Patch
> 
> The code change  looks good. We just need to add unit tests.

Added some.

(In reply to Darin Adler from comment #3)
> Comment on attachment 319953 [details]
> Patch
> 
> Why does this have to be a named function rather than a constructor?

It would be a bit nicer to use but it conflicts with other constructors.
There might be ways to disambiguate or restrict this constructor but I am not sure this is worth it.
I wonder whether latest C++ progress might help.

(In reply to Darin Adler from comment #4)
> Comment on attachment 319953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319953&action=review
> 
> > Source/WTF/wtf/Vector.h:632
> > +        result.asanSetInitialBufferSizeTo(size);
> 
> I don’t understand why this is needed. The reserveInitialCapacity call
> should take care of this.

Right, changed here and in initializer list constructor.

> 
> > Source/WTF/wtf/Vector.h:810
> > +        uncheckedAppend(std::forward<U>(item));
> > +        uncheckedAppend(std::forward<Items>(items)...);
> 
> Seems like we miss an opportunity here to optimize so we bump the size only
> once. This is going to increment the size over and over again, I think. Need
> to look at the generated code I guess.

Added the optimization

[Sam Weinig]

Comment on attachment 320037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320037&action=review

> Tools/ChangeLog:8
> +        fix-176432

This could be a better comment. :)

[Saam Barati]

Comment on attachment 320037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320037&action=review

> Source/WTF/wtf/Vector.h:817
> +        auto ptr = std::addressof(value);
> +        new (NotNull, begin() + position) T(std::forward<U>(*ptr));

Why is this needed instead of just:
new (NotNull, begin() + position) T(std::forward<U>(value));

[youenn fablet]

(In reply to Saam Barati from comment #8)
> Comment on attachment 320037 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320037&action=review
> 
> > Source/WTF/wtf/Vector.h:817
> > +        auto ptr = std::addressof(value);
> > +        new (NotNull, begin() + position) T(std::forward<U>(*ptr));
> 
> Why is this needed instead of just:
> new (NotNull, begin() + position) T(std::forward<U>(value));

I am not sure WebKit is using Vector of classes with overloaded &.
Anders might know more about this since he added it.

[Anders Carlsson]

(In reply to youenn fablet from comment #9)
> (In reply to Saam Barati from comment #8)
> > Comment on attachment 320037 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=320037&action=review
> > 
> > > Source/WTF/wtf/Vector.h:817
> > > +        auto ptr = std::addressof(value);
> > > +        new (NotNull, begin() + position) T(std::forward<U>(*ptr));
> > 
> > Why is this needed instead of just:
> > new (NotNull, begin() + position) T(std::forward<U>(value));
> 
> I am not sure WebKit is using Vector of classes with overloaded &.
> Anders might know more about this since he added it.

COMPtr has an overloaded & operator, and I had planned to add one to RetainPtr as well, since a lot of the CF APIs use out-pointers.

[Saam Barati]

(In reply to youenn fablet from comment #9)
> (In reply to Saam Barati from comment #8)
> > Comment on attachment 320037 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=320037&action=review
> > 
> > > Source/WTF/wtf/Vector.h:817
> > > +        auto ptr = std::addressof(value);
> > > +        new (NotNull, begin() + position) T(std::forward<U>(*ptr));
> > 
> > Why is this needed instead of just:
> > new (NotNull, begin() + position) T(std::forward<U>(value));
> 
> I am not sure WebKit is using Vector of classes with overloaded &.
> Anders might know more about this since he added it.

Right, that makes sense. But why do we even bother getting the address then dereferencing it? Why can't we just forward it along? Maybe I'm missing something

[youenn fablet]

> Right, that makes sense. But why do we even bother getting the address then
> dereferencing it? Why can't we just forward it along? Maybe I'm missing
> something

I read too fast, you are right, I will simplify uncheckedAppend and uncheckedInitialize accordingly.

[youenn fablet]

Created attachment 320563 [details]
Patch

[Saam Barati]

Comment on attachment 320563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320563&action=review

> Source/WebKit/ChangeLog:3
> +        Vector should be able to easily create from a list of movable only items

I think the name of this bug is slightly misleading. At least I misread it in my head. I don't think you're not requiring things that are rvalue references here. It looks like you're just using normal forwarding semantics.

> Source/WTF/wtf/Vector.h:1185
> +    asanSetInitialBufferSizeTo(initialCapacity);

This seems wrong if the above branch is not taken.
Also, I think this would break asan finding bugs like:
```
Vector<T, 0> v;
v.reserveInitialCapacity(10);
v[0] = T();
```

[Saam Barati]

Comment on attachment 320563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320563&action=review

> Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:111
> +TEST(WTF_Vector, ConstructWithFromMoveOnly)

I think it's worth adding tests for non rvalue references.

[youenn fablet]

Created attachment 320959 [details]
Patch

[youenn fablet]

Thanks Saam, I updated the patch according your comments.

As of the name of the bug, the issue was brought up for Movable only types although you are right this routine is applicable to more than that.

[Saam Barati]

(In reply to youenn fablet from comment #16)
> Created attachment 320959 [details]
> Patch

Did you want this patch reviewed?

[youenn fablet]

(In reply to Saam Barati from comment #18)
> (In reply to youenn fablet from comment #16)
> > Created attachment 320959 [details]
> > Patch
> 
> Did you want this patch reviewed?

After some refactoring, I no longer need this.
It makes sense to have it in bugzilla if somebody in the future needs that but I am not sure we should land it.

[youenn fablet]

I have a use case for it.

[youenn fablet]

Created attachment 323282 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 323282 [details]
Patch for landing

Clearing flags on attachment: 323282

Committed r223122: <http://trac.webkit.org/changeset/223122>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34903219>