Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1505773002:
Teach WTF::Vector to work with move-only types (Closed)
DescriptionTeach WTF::Vector to work with move-only types
The unit tests were added as well.
BUG=567139
Committed: https://crrev.com/75c7843dcb738620b94332bc4d744dcdf38dbdec
Cr-Commit-Position: refs/heads/master@{#366966}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Comments from Yuta Kitamura #Patch Set 3 : Fix crashes #
Total comments: 8
Patch Set 4 : review comments #
Total comments: 14
Patch Set 5 : VectorTest.MoveOnlyType fixes #
Total comments: 1
Patch Set 6 : Comments from danakj #
Total comments: 6
Patch Set 7 : Comments from Nico #
Messages
Total messages: 39 (10 generated)
Description was changed from ========== Teach WTF::Vector to work with move-only types The std::unique_ptr has been taken as an example in the unit tests. BUG=567139 ========== to ========== Teach WTF::Vector to work with move-only types The std::unique_ptr has been taken as an example in the unit tests. BUG=567139 ==========
mikhail.pozdnyakov@intel.com changed reviewers: + thakis@chromium.org
yutak@chromium.org changed reviewers: + yutak@chromium.org
Before we land this, we need to consider what we should do for WTF::forward(). Rvalue references are not allowed in Chromium yet. Move constructor/assignments are allowed in Chromium, but they need to be discussed on blink-dev before we make heavy use in Blink. (Existing move ctor/assign in Vector is something we derived from WebKit.) std::unique_ptr<> should NOT be used in Blink. It is too confusing while we have OwnPtr/PassOwnPtr. I don't think we should land this for now.
On 2015/12/08 06:44:01, Yuta Kitamura wrote: > Before we land this, we need to consider what we should do > for WTF::forward(). This CL is an example of how std::forward can be used in blink. https://groups.google.com/a/chromium.org/forum/#!topic/cxx/-O7euklhSxs I do not see any reason for using WTF::forward instead > > Rvalue references are not allowed in Chromium yet. > Move constructor/assignments are allowed in Chromium, but > they need to be discussed on blink-dev before we make > heavy use in Blink. (Existing move ctor/assign in Vector is > something we derived from WebKit.) Judging by http://chromium-cpp.appspot.com/ both rvalue references and std::move are allowed. > > std::unique_ptr<> should NOT be used in Blink. It is too > confusing while we have OwnPtr/PassOwnPtr. unique_ptr could substitute OwnPtr/PassOwnPtr, however so far this is just a test and I can re-make it to using a mock MoveOnly type. > > I don't think we should land this for now. Even now it can improve code path for example for 'Vector<String>'
On 2015/12/09 13:37:19, Mikhail wrote: > On 2015/12/08 06:44:01, Yuta Kitamura wrote: > > Before we land this, we need to consider what we should do > > for WTF::forward(). > > This CL is an example of how std::forward can be used in blink. > https://groups.google.com/a/chromium.org/forum/#!topic/cxx/-O7euklhSxs No I meant WTF::forward. > > I do not see any reason for using WTF::forward instead > > > > Rvalue references are not allowed in Chromium yet. > > Move constructor/assignments are allowed in Chromium, but > > they need to be discussed on blink-dev before we make > > heavy use in Blink. (Existing move ctor/assign in Vector is > > something we derived from WebKit.) > Judging by http://chromium-cpp.appspot.com/ both rvalue references and std::move > are allowed. Again that is for Chromium and not for Blink. Blink's C++ usage is different from Chromium's, and features allowed in Chromium does not universally apply to Blink. > > > > > std::unique_ptr<> should NOT be used in Blink. It is too > > confusing while we have OwnPtr/PassOwnPtr. > > unique_ptr could substitute OwnPtr/PassOwnPtr, however so far this is just a > test and I can re-make it to using a mock MoveOnly type. Allowing some C++ constructs only in tests is too confusing and I don't think we should allow this kind of usage. > > > > I don't think we should land this for now. > Even now it can improve code path for example for 'Vector<String>' Moving in Blink is an important topic and needs to be discussed in blink-dev first. NOT LGTM for now.
On 2015/12/10 02:33:22, Yuta Kitamura wrote: > On 2015/12/09 13:37:19, Mikhail wrote: > > On 2015/12/08 06:44:01, Yuta Kitamura wrote: > > > Before we land this, we need to consider what we should do > > > for WTF::forward(). > > > > This CL is an example of how std::forward can be used in blink. > > https://groups.google.com/a/chromium.org/forum/#!topic/cxx/-O7euklhSxs > > No I meant WTF::forward. > > > > > I do not see any reason for using WTF::forward instead > > > > > > Rvalue references are not allowed in Chromium yet. > > > Move constructor/assignments are allowed in Chromium, but > > > they need to be discussed on blink-dev before we make > > > heavy use in Blink. (Existing move ctor/assign in Vector is > > > something we derived from WebKit.) > > Judging by http://chromium-cpp.appspot.com/ both rvalue references and > std::move > > are allowed. > > Again that is for Chromium and not for Blink. Blink's C++ usage is > different from Chromium's, and features allowed in Chromium does > not universally apply to Blink. > I see. Then do we have a list of C++11 features allowed particularly for Blink (and a defined procedure for adding new features)? and if not isn't it worth creating?
On 2015/12/10 09:25:28, Mikhail wrote: > On 2015/12/10 02:33:22, Yuta Kitamura wrote: > > On 2015/12/09 13:37:19, Mikhail wrote: > > > On 2015/12/08 06:44:01, Yuta Kitamura wrote: > > > > Before we land this, we need to consider what we should do > > > > for WTF::forward(). > > > > > > This CL is an example of how std::forward can be used in blink. > > > https://groups.google.com/a/chromium.org/forum/#!topic/cxx/-O7euklhSxs > > > > No I meant WTF::forward. > > > > > > > > I do not see any reason for using WTF::forward instead > > > > > > > > Rvalue references are not allowed in Chromium yet. > > > > Move constructor/assignments are allowed in Chromium, but > > > > they need to be discussed on blink-dev before we make > > > > heavy use in Blink. (Existing move ctor/assign in Vector is > > > > something we derived from WebKit.) > > > Judging by http://chromium-cpp.appspot.com/ both rvalue references and > > std::move > > > are allowed. > > > > Again that is for Chromium and not for Blink. Blink's C++ usage is > > different from Chromium's, and features allowed in Chromium does > > not universally apply to Blink. > > > I see. Then do we have a list of C++11 features allowed particularly for Blink > (and a defined procedure for adding new features)? and if not isn't it worth > creating? I'm planning to create a list and announce it to blink-dev sometime next week.
I say LGTM just to cancel my red bit, but you really need to fix the const_cast stuff. Your code is wrong. Before proceeding to land this patch, - Take a look at the discussion held at https://groups.google.com/a/chromium.org/forum/#!forum/platform-architecture-dev - Get the patch reviewed from someone involved in the above discussion for sanity check. https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Vector.h:1161: auto ptr = const_cast<typename std::remove_const<typename std::remove_reference<U>::type>::type*>(&val); This const_cast dance is not necessary at all. Your code just obfuscates things and yields less readability. You would just need: typename std::remove_reference<U>::type* ptr = &val; https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Vector.h:1183: auto ptr = const_cast<typename std::remove_const<typename std::remove_reference<U>::type>::type*>(&val); Ditto. https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Vector.h:1221: auto data = const_cast<typename std::remove_const<typename std::remove_reference<U>::type>::type*>(&val); Ditto.
https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Vector.h:1146: ANNOTATE_CHANGE_SIZE(begin(), capacity(), m_size, m_size + 1); Remove the unnecessary spaces.
And also, check-webkit-style must be fixed before landing (I think presubmit errors will block landing).
https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Vector.h:1146: ANNOTATE_CHANGE_SIZE(begin(), capacity(), m_size, m_size + 1); On 2015/12/15 10:45:32, Yuta Kitamura wrote: > Remove the unnecessary spaces. Done. https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Vector.h:1161: auto ptr = const_cast<typename std::remove_const<typename std::remove_reference<U>::type>::type*>(&val); On 2015/12/15 10:43:53, Yuta Kitamura wrote: > This const_cast dance is not necessary at all. Your code just > obfuscates things and yields less readability. > > You would just need: > > typename std::remove_reference<U>::type* ptr = &val; done. Thanks for pointing out. https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Vector.h:1183: auto ptr = const_cast<typename std::remove_const<typename std::remove_reference<U>::type>::type*>(&val); On 2015/12/15 10:43:53, Yuta Kitamura wrote: > Ditto. Done. https://codereview.chromium.org/1505773002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Vector.h:1221: auto data = const_cast<typename std::remove_const<typename std::remove_reference<U>::type>::type*>(&val); On 2015/12/15 10:43:53, Yuta Kitamura wrote: > Ditto. Done.
On 2015/12/15 10:43:53, Yuta Kitamura wrote: > I say LGTM just to cancel my red bit, but you really need > to fix the const_cast stuff. Your code is wrong. > > Before proceeding to land this patch, > > - Take a look at the discussion held at > https://groups.google.com/a/chromium.org/forum/#!forum/platform-architecture-dev > - Get the patch reviewed from someone involved in the above > discussion for sanity check. > Thanks for the link and it's great that now we've a defined list of allowed C++11 features in blink, maybe it's also worth advertising in blink-dev ML
mikhail.pozdnyakov@intel.com changed reviewers: + esprehn@chromium.org
On 2015/12/15 11:12:12, Mikhail wrote: > On 2015/12/15 10:43:53, Yuta Kitamura wrote: > > I say LGTM just to cancel my red bit, but you really need > > to fix the const_cast stuff. Your code is wrong. > > > > Before proceeding to land this patch, > > > > - Take a look at the discussion held at > > > https://groups.google.com/a/chromium.org/forum/#!forum/platform-architecture-dev > > - Get the patch reviewed from someone involved in the above > > discussion for sanity check. > > > Thanks for the link and it's great that now we've a defined list of allowed > C++11 features in blink, maybe it's also worth advertising in blink-dev ML Yep, I just need to apply people's comments and will send it out soon.
There seems to be some crashes that both of us failed to address. PluginPowerSaverBrowserTest.* looks like an obvious example.
On 2015/12/15 12:05:07, Yuta Kitamura wrote: > There seems to be some crashes that both of us failed to > address. PluginPowerSaverBrowserTest.* looks like an > obvious example. Should be fixed now.
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Vector.h:748: template <typename U> void append(U&&); It's interesting that Vector has templated methods like append() here, which will make a different append() method for every callsite of different convertable types. std::vector by comparison has push_back(const T&) and push_back(T&&) but with T from the *class template*, and the method doesn't itself add any new template types. I think(?) we'd get smaller code if we did the same thing here.. but that's how it was before this CL I see. https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Vector.h:1183: typename std::remove_reference<U>::type* ptr = &val; What is the point of this? Why not just forward(val)? https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:231: TEST(VectorTest, unique_ptr) I'd prefer if you used your own custom move-only type in the unit test. We're going to add special case stuff for unique_ptr to make it move with memcpy and such at some point. (It might be nice to test both with and without the various Vector optimizations actually for move-only types: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Vector.h:748: template <typename U> void append(U&&); On 2015/12/15 at 22:35:23, danakj (behind on reviews) wrote: > It's interesting that Vector has templated methods like append() here, which will make a different append() method for every callsite of different convertable types. > > std::vector by comparison has push_back(const T&) and push_back(T&&) but with T from the *class template*, and the method doesn't itself add any new template types. > > I think(?) we'd get smaller code if we did the same thing here.. but that's how it was before this CL I see. https://chromium.googlesource.com/chromium/src/+/7d5001c28efd6f589bd5ac468f2e... This was to do better conversions, for example from PassRefPtr to RefPtr without churn. We lost the comment a very long time ago, and with move semantics perhaps we can remove this now. https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:231: TEST(VectorTest, unique_ptr) On 2015/12/15 at 22:35:23, danakj (behind on reviews) wrote: > I'd prefer if you used your own custom move-only type in the unit test. We're going to add special case stuff for unique_ptr to make it move with memcpy and such at some point. > > (It might be nice to test both with and without the various Vector optimizations actually for move-only types: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) +1, lets not use unique_ptr here.
https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Vector.h:1183: typename std::remove_reference<U>::type* ptr = &val; On 2015/12/15 22:35:23, danakj (behind on reviews) wrote: > What is the point of this? Why not just forward(val)? Yeah, I think just forward(val) is sufficient. (In my patch I did that.)
https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Vector.h:1183: typename std::remove_reference<U>::type* ptr = &val; On 2015/12/15 22:35:23, danakj (behind on reviews) wrote: > What is the point of this? Why not just forward(val)? Done. https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:231: TEST(VectorTest, unique_ptr) On 2015/12/15 23:46:47, esprehn wrote: > On 2015/12/15 at 22:35:23, danakj (behind on reviews) wrote: > > I'd prefer if you used your own custom move-only type in the unit test. We're > going to add special case stuff for unique_ptr to make it move with memcpy and > such at some point. > > > > (It might be nice to test both with and without the various Vector > optimizations actually for move-only types: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) > > +1, lets not use unique_ptr here. Done.
https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:268: ASSERT_EQ(1, first.value()); nit: just vector.first().value() here? https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:272: for (auto iter = vector.begin(); iter != vector.end(); ++iter) { What is this loop adding that check first and last didn't already cover (that is specific to move-only types)? https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:277: for (size_t index = 0; index < vector.size(); ++index) { ditto https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:282: EXPECT_EQ(1, vector[0].value()); ditto https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:293: size_t count = 1025; why 1025? does this cause reallocs? can you verify/prove/force that in the test somehow? nit: use int not size_t here https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:302: EXPECT_EQ(0u, vector.size()); it's not actually valid to use |vector| after you have moved from it for anything other than assigning to it.
https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:302: EXPECT_EQ(0u, vector.size()); On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > it's not actually valid to use |vector| after you have moved from it for > anything other than assigning to it. So, let me half take this back. It's fine if WTF::Vector provides some contract here. But currently what WTF::Vector does is swap() with the other vector, which doesn't appear to me like the kind of contract I'd expect, and would consider the other vector to be considered unusable at that point.
https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:268: ASSERT_EQ(1, first.value()); On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > nit: just vector.first().value() here? Done. https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:272: for (auto iter = vector.begin(); iter != vector.end(); ++iter) { On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > What is this loop adding that check first and last didn't already cover (that is > specific to move-only types)? Just iterator semantics check https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:277: for (size_t index = 0; index < vector.size(); ++index) { On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > ditto check of [] operator, but can be removed as it's checked below https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:293: size_t count = 1025; On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > why 1025? does this cause reallocs? can you verify/prove/force that in the test > somehow? I took after VectorTest.OwnPtr, however 'vector.capacity() + 1' suffices. > > nit: use int not size_t here that's because vector methods use this type, so we avoid extra casting inside EXPECT_EQ https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:302: EXPECT_EQ(0u, vector.size()); On 2015/12/16 21:15:12, danakj (behind on reviews) wrote: > On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > > it's not actually valid to use |vector| after you have moved from it for > > anything other than assigning to it. > > So, let me half take this back. It's fine if WTF::Vector provides some contract > here. But currently what WTF::Vector does is swap() with the other vector, which > doesn't appear to me like the kind of contract I'd expect, and would consider > the other vector to be considered unusable at that point. Done.
Thanks, non-owner LGTM w/ a couple last comments. https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:272: for (auto iter = vector.begin(); iter != vector.end(); ++iter) { On 2015/12/18 13:26:41, Mikhail wrote: > On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > > What is this loop adding that check first and last didn't already cover (that > is > > specific to move-only types)? > > Just iterator semantics check My question was more that your patch is adding move support to Vector, and that didn't involve changing iterators or operator[]. Those things are testing by other tests, and there's nothing move-only-specific about them. It's not terrible, but I like tests to be as focused as possible rather than copy/paste and each test tests everything. https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:302: EXPECT_EQ(0u, vector.size()); On 2015/12/18 13:26:41, Mikhail wrote: > On 2015/12/16 21:15:12, danakj (behind on reviews) wrote: > > On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > > > it's not actually valid to use |vector| after you have moved from it for > > > anything other than assigning to it. > > > > So, let me half take this back. It's fine if WTF::Vector provides some > contract > > here. But currently what WTF::Vector does is swap() with the other vector, > which > > doesn't appear to me like the kind of contract I'd expect, and would consider > > the other vector to be considered unusable at that point. > > Done. Thanks this is now valid, though you might wanna actually test moving the vector? You just want to verify the moved-into vector after, not the moved-from one. https://codereview.chromium.org/1505773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/VectorTest.cpp:290: // Rellocation did not affect the vector's content. "Reallocation"?
On 2015/12/18 19:05:50, danakj (behind on reviews) wrote: > Thanks, non-owner LGTM w/ a couple last comments. > > https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/wtf/VectorTest.cpp (right): > > https://codereview.chromium.org/1505773002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/wtf/VectorTest.cpp:272: for (auto iter = > vector.begin(); iter != vector.end(); ++iter) { > On 2015/12/18 13:26:41, Mikhail wrote: > > On 2015/12/16 21:10:34, danakj (behind on reviews) wrote: > > > What is this loop adding that check first and last didn't already cover > (that > > is > > > specific to move-only types)? > > > > Just iterator semantics check > > My question was more that your patch is adding move support to Vector, and that > didn't involve changing iterators or operator[]. Those things are testing by > other tests, and there's nothing move-only-specific about them. It's not > terrible, but I like tests to be as focused as possible rather than copy/paste > and each test tests everything. > I've dropped the unnecessary checks. > Thanks this is now valid, though you might wanna actually test moving the > vector? You just want to verify the moved-into vector after, not the moved-from > one. Added a vector moving check. > > https://codereview.chromium.org/1505773002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/wtf/VectorTest.cpp (right): > > https://codereview.chromium.org/1505773002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/wtf/VectorTest.cpp:290: // Rellocation did not affect > the vector's content. > "Reallocation"? fixed, thanks.
lgtm with comments, thanks! https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Vector.h:757: template <typename U> void prepend(U&&); huh, why do we have a prepend() method on vector, that's weird https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Vector.h:1161: typename std::remove_reference<U>::type* ptr = &val; This does the right thing for a test case like in https://github.com/WebKit/webkit/commit/b3be6e15bf55eeccaea6b056379372f090f91d12 , yes? https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/VectorTest.cpp:190: for (index = 0; index < vector.size(); ++index) { Please use trailing ++ in this file, the file is currently self-consistent
https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Vector.h:1161: typename std::remove_reference<U>::type* ptr = &val; On 2015/12/22 15:43:23, Nico wrote: > This does the right thing for a test case like in > https://github.com/WebKit/webkit/commit/b3be6e15bf55eeccaea6b056379372f090f91d12 > , yes? Unfortunately not, fixed in the latest patch. Thanks for pointing it out. https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/VectorTest.cpp:190: for (index = 0; index < vector.size(); ++index) { On 2015/12/22 15:43:23, Nico wrote: > Please use trailing ++ in this file, the file is currently self-consistent Done.
lgtm if I guessed the motivation correctly (or with a different good motivation :-) ) https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Vector.h (right): https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Vector.h:1161: typename std::remove_reference<U>::type* ptr = &val; On 2015/12/23 11:59:23, Mikhail wrote: > On 2015/12/22 15:43:23, Nico wrote: > > This does the right thing for a test case like in > > > https://github.com/WebKit/webkit/commit/b3be6e15bf55eeccaea6b056379372f090f91d12 > > , yes? > > Unfortunately not, fixed in the latest patch. Thanks for pointing it out. Thanks. Anything wrong with the fix in https://github.com/WebKit/webkit/commit/b3be6e15bf55eeccaea6b056379372f090f91d12 (instead of adding another expandCapacity() overload)? I suppose this is a bit safer if there are other callers of expandCapacity() that aren't careful either?
Description was changed from ========== Teach WTF::Vector to work with move-only types The std::unique_ptr has been taken as an example in the unit tests. BUG=567139 ========== to ========== Teach WTF::Vector to work with move-only types The unit tests were added as well. BUG=567139 ==========
On 2015/12/23 21:50:07, Nico wrote: > lgtm if I guessed the motivation correctly (or with a different good motivation > :-) ) > > https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/wtf/Vector.h (right): > > https://codereview.chromium.org/1505773002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/wtf/Vector.h:1161: typename > std::remove_reference<U>::type* ptr = &val; > On 2015/12/23 11:59:23, Mikhail wrote: > > On 2015/12/22 15:43:23, Nico wrote: > > > This does the right thing for a test case like in > > > > > > https://github.com/WebKit/webkit/commit/b3be6e15bf55eeccaea6b056379372f090f91d12 > > > , yes? > > > > Unfortunately not, fixed in the latest patch. Thanks for pointing it out. > > Thanks. Anything wrong with the fix in > https://github.com/WebKit/webkit/commit/b3be6e15bf55eeccaea6b056379372f090f91d12 > (instead of adding another expandCapacity() overload)? I suppose this is a bit > safer if there are other callers of expandCapacity() that aren't careful either? I like the overload approach more for better readability.
The CQ bit was checked by mikhail.pozdnyakov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1505773002/#ps120001 (title: "Comments from Nico")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1505773002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1505773002/120001
Message was sent while issue was closed.
Description was changed from ========== Teach WTF::Vector to work with move-only types The unit tests were added as well. BUG=567139 ========== to ========== Teach WTF::Vector to work with move-only types The unit tests were added as well. BUG=567139 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Teach WTF::Vector to work with move-only types The unit tests were added as well. BUG=567139 ========== to ========== Teach WTF::Vector to work with move-only types The unit tests were added as well. BUG=567139 Committed: https://crrev.com/75c7843dcb738620b94332bc4d744dcdf38dbdec Cr-Commit-Position: refs/heads/master@{#366966} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/75c7843dcb738620b94332bc4d744dcdf38dbdec Cr-Commit-Position: refs/heads/master@{#366966} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
