Vector should be able to easily create from a list of movable only items
Created attachment 319953 [details] Patch
Comment on attachment 319953 [details] Patch The code change looks good. We just need to add unit tests.
Comment on attachment 319953 [details] Patch Why does this have to be a named function rather than a constructor?
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.
Created attachment 320037 [details] Patch
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
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. :)
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));
(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.
(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.
(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
> 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.
Created attachment 320563 [details] Patch
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(); ```
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.
Created attachment 320959 [details] Patch
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.
(In reply to youenn fablet from comment #16) > Created attachment 320959 [details] > Patch Did you want this patch reviewed?
(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.
I have a use case for it.
Created attachment 323282 [details] Patch for landing
Comment on attachment 323282 [details] Patch for landing Clearing flags on attachment: 323282 Committed r223122: <http://trac.webkit.org/changeset/223122>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34903219>