Bug 177732 – There should be a version of copyToVector that returns a Vector, rather than using an out parameter

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Sam Weinig]

As Darin noted in 177728, it is quite lame that copyToVector (and its ilk) uses an out parameter and doesn't just return a Vector. I think we can leverage the WTF::map to make this rather simple. Perhaps add to Vector.h something like:

template<typename T>
static T mapIdentity(const T& value)
{
    return WTFMove(value);
}

template<typename C>
inline auto copyToVector(const C& collection) -> decltype(map(collection, mapIdentity))
{
    return map(collection, mapIdentity);
}

It would also be nice to make things like copyToVector(hashmap.keys()) work, rather than having separate copyKeysToVector/copyValuesToVector functions. I think that could be done by adding a new SizedIteratorRange class that keys() and values() return (rather than the IteratorRange they currently return) so they can work with WTF::map.

[Ryosuke Niwa]

Or Vector::from to match Array.from?

[Ryosuke Niwa]

Yet another option is to make Vector constructor work with whatever iterable type with size() so that we can just do Vector<>(collection).

[Sam Weinig]

Another option would be to make WTF::map default to the identity function for its mapping, so that we can write copyToVector as just WTF::map(collection). Thought that may be a bit strange to read at call sites.

[Chris Dumez]

The templated out parameter is convenient for when you want to copy to a vector that contains items of another type, which happens fairly commonly.

E.g. you have a HashSet<Node*> and want to copy to a Vector<RefPtr<Node>>.

Not sure how we can make this work:

HashSet<Node*> nodes;
Vector<RefPtr<Node>> nodesCopy = copyToVector(nodes);

[Darin Adler]

(In reply to Chris Dumez from comment #4)
> The templated out parameter is convenient for when you want to copy to a
> vector that contains items of another type, which happens fairly commonly.
> 
> E.g. you have a HashSet<Node*> and want to copy to a Vector<RefPtr<Node>>.
> 
> Not sure how we can make this work:
> 
> HashSet<Node*> nodes;
> Vector<RefPtr<Node>> nodesCopy = copyToVector(nodes);

Using your suggested function name, copyToVector, the syntax I would suggest would be:

    auto nodesCopy = copyToVector<RefPtr<Node>>(nodes);

Should not be too hard to implement.

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> (In reply to Chris Dumez from comment #4)
> > The templated out parameter is convenient for when you want to copy to a
> > vector that contains items of another type, which happens fairly commonly.
> > 
> > E.g. you have a HashSet<Node*> and want to copy to a Vector<RefPtr<Node>>.
> > 
> > Not sure how we can make this work:
> > 
> > HashSet<Node*> nodes;
> > Vector<RefPtr<Node>> nodesCopy = copyToVector(nodes);
> 
> Using your suggested function name, copyToVector, the syntax I would suggest
> would be:
> 
>     auto nodesCopy = copyToVector<RefPtr<Node>>(nodes);
> 
> Should not be too hard to implement.

Sure, if you add another template parameter then we can make it work. It is more verbose though.

[Darin Adler]

Maybe we can build this with std::size?

http://en.cppreference.com/w/cpp/iterator/size
http://en.cppreference.com/w/cpp/iterator/begin
http://en.cppreference.com/w/cpp/iterator/end

[Sam Weinig]

Created attachment 322431 [details]
Patch to test

[Sam Weinig]

Added a version that supports both

1) 
  HashSet<int> intSet { 1, 2, 3 };
  auto intVector = copyToVector(intSet);

2)
  HashSet<int> intSet { 1, 2, 3 };
  auto floatVector = copyToVector<float>(intSet);

[Build Bot]

Attachment 322431 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Vector.h:1613:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Vector.h:1618:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Vector.h:1620:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Vector.h:1625:  is_iterable_impl is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/Vector.h:1645:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Vector.h:1646:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 6 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322488 [details]
Another version

Here is another version that doesn't require the gross is_iterable stuff.

[Build Bot]

Attachment 322488 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Vector.h:1611:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Vector.h:1635:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WTF/wtf/Vector.h:1636:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Vector.h:1637:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/Vector.h:1642:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 322488 [details]
Another version

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

> Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:782
> +    auto vector = copyToVector(intSet);

Maybe we should call this makeVector since we're not really coping to some existing vector?

[Sam Weinig]

(In reply to Sam Weinig from comment #11)
> Created attachment 322488 [details]
> Another version
> 
> Here is another version that doesn't require the gross is_iterable stuff.

Try as I may, I can't get the overloads to work in all compilers right now. So it may have to be:

template<typename C>
inline auto copyToVector(const C& collection) -> decltype(map(collection, mapIdentity))
{
    return map(collection, mapIdentity);

template<typename ResultType, typename C>
inline auto copyTo(const C& collection) -> ResultType
{
    return map(collection, mapIdentity);
}

or maybe

template<typename ResultItem, typename C>
inline auto copyToVectorOf(const C& collection) -> Vector<ResultType>
{
    return map(collection, mapIdentity);
}


which would give use the normal kind,

    auto nodesCopy = copyToVector(nodes);

and two options for the other one, either:

    auto nodesCopy = copyTo<Vector<RefPtr<Node>>>(nodes);

or:

    auto nodesCopy = copyToVectorOf<RefPtr<Node>>(nodes);


(In reply to Ryosuke Niwa from comment #13)
> Comment on attachment 322488 [details]
>
> Maybe we should call this makeVector since we're not really coping to some
> existing vector?

I like having the word copy in it since this is not going to attempt to move.

[Sam Weinig]

Created attachment 323134 [details]
Patch

[Build Bot]

Attachment 323134 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Vector.h:1612:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Vector.h:1613:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/Vector.h:1619:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 323134 [details]
Patch

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

r=me

> Source/WTF/wtf/Vector.h:1614
> +template<typename Collection>
> +inline auto copyToVector(const Collection& collection) -> Vector<typename MapFunctionInspector<Collection>::SourceItemType>
> +{
> +    return WTF::map(collection, [](const auto& v) { return v; });
> +}

I think you could also write this as calling copyToVectorOf, maybe that's a bit cleaner?

> Source/WTF/wtf/Vector.h:1619
> +    return WTF::map(collection, [](const auto& v) -> DestinationItemType { return v; });

I never know what official WebKit style is, but I know in JSC we tend to put a space between "]" and "(" for lambdas.

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

Is it worth adding a test where we do side effects in the copy constructor?

I think it's worth having a test where the input is a vector, and then we can ensure the resulting vector has the correct order, etc.

[Darin Adler]

Comment on attachment 323134 [details]
Patch

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

>> Source/WTF/wtf/Vector.h:1619
>> +    return WTF::map(collection, [](const auto& v) -> DestinationItemType { return v; });
> 
> I never know what official WebKit style is, but I know in JSC we tend to put a space between "]" and "(" for lambdas.

The only thing that makes WebKit style “official” is when we discuss it together. I am pretty sure we do put spaces after the "]" in lambdas. Someone can add it to the style guide document so we don’t forget or get confused.

>> Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:792
>> +TEST(WTF_Vector, CopyToVectorOf)
> 
> Is it worth adding a test where we do side effects in the copy constructor?
> 
> I think it's worth having a test where the input is a vector, and then we can ensure the resulting vector has the correct order, etc.

Could do that with ListHashSet.

[Sam Weinig]

(In reply to Saam Barati from comment #17)
> Comment on attachment 323134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323134&action=review
> 
> r=me
> 
> > Source/WTF/wtf/Vector.h:1614
> > +template<typename Collection>
> > +inline auto copyToVector(const Collection& collection) -> Vector<typename MapFunctionInspector<Collection>::SourceItemType>
> > +{
> > +    return WTF::map(collection, [](const auto& v) { return v; });
> > +}
> 
> I think you could also write this as calling copyToVectorOf, maybe that's a
> bit cleaner?

I tried that, but it ends up being quite a bit longer and harder to read, due to repeating the MapFunctionInspector stuff to get the item type.

> 
> > Source/WTF/wtf/Vector.h:1619
> > +    return WTF::map(collection, [](const auto& v) -> DestinationItemType { return v; });
> 
> I never know what official WebKit style is, but I know in JSC we tend to put
> a space between "]" and "(" for lambdas.

Will update.

> 
> > Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:792
> > +TEST(WTF_Vector, CopyToVectorOf)
> 
> Is it worth adding a test where we do side effects in the copy constructor?
> 
> I think it's worth having a test where the input is a vector, and then we
> can ensure the resulting vector has the correct order, etc.

Sure. I like Darin's suggestion of ListHashSet since that is more realistic.

[Sam Weinig]

Created attachment 323150 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #20)
> Created attachment 323150 [details]
> Patch

I re-uploaded it for review, since I made some substantial changes to get ListHashSet to work (apparently it's iterator is a bit more const-y than HashSet). I ended up using Saam's suggestion of basing copyToVector on copyToVectorOf.

[Build Bot]

Attachment 323150 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Vector.h:1613:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/Vector.h:1623:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 323150 [details]
Patch

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

Are you going to make this work for map keys and values?

> Source/WTF/wtf/Vector.h:1633
>  using WTF::removeRepeatedElements;
> +using WTF::copyToVector;
> +using WTF::copyToVectorOf;

We usually sort these alphabetically.

[WebKit Commit Bot]

Comment on attachment 323150 [details]
Patch

Clearing flags on attachment: 323150

Committed r223039: <http://trac.webkit.org/changeset/223039>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34878508>

[Sam Weinig]

(In reply to Darin Adler from comment #23)
> Comment on attachment 323150 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323150&action=review
> 
> Are you going to make this work for map keys and values?
> 
> > Source/WTF/wtf/Vector.h:1633
> >  using WTF::removeRepeatedElements;
> > +using WTF::copyToVector;
> > +using WTF::copyToVectorOf;
> 
> We usually sort these alphabetically.

Addressed in r223040.