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.
Or Vector::from to match Array.from?
Yet another option is to make Vector constructor work with whatever iterable type with size() so that we can just do Vector<>(collection).
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.
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);
(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.
(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.
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
Created attachment 322431 [details] Patch to test
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);
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.
Created attachment 322488 [details] Another version Here is another version that doesn't require the gross is_iterable stuff.
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.
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?
(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.
Created attachment 323134 [details] Patch
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.
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.
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.
(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.
Created attachment 323150 [details] Patch
(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.
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.
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.
Comment on attachment 323150 [details] Patch Clearing flags on attachment: 323150 Committed r223039: <http://trac.webkit.org/changeset/223039>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34878508>
(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.