Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163033002:
base::Version constructor from std::vector<uint32_t> (Closed)
Descriptionbase::Version constructor from std::vector<uint32_t>
The new constructor is to be used by Version's mojom struct and its type mapping.
BUG=622707
Committed: https://crrev.com/8fb38080e8833699973f510470fe2664e4fd3df6
Cr-Commit-Position: refs/heads/master@{#407529}
Patch Set 1 #
Total comments: 5
Patch Set 2 : operator= and changes based on comments #
Total comments: 12
Patch Set 3 : Vector passed by value #
Total comments: 2
Patch Set 4 : Changes based on nit comments #
Messages
Total messages: 29 (14 generated)
staraz@chromium.org changed reviewers: + danakj@chromium.org, fsamuel@chromium.org
Please review my changes. I added a move constructor to base::Version to be used by mojo.common.mojom.Version added in https://codereview.chromium.org/2147693002/
https://codereview.chromium.org/2163033002/diff/1/base/version.cc File base/version.cc (right): https://codereview.chromium.org/2163033002/diff/1/base/version.cc#newcode97 base/version.cc:97: components_ = components; Add to initializer list.
https://codereview.chromium.org/2163033002/diff/1/base/version.h File base/version.h (right): https://codereview.chromium.org/2163033002/diff/1/base/version.h#newcode35 base/version.h:35: explicit Version(std::vector<uint32_t>&& components); this should come with an operator=. This also deletes the implicit copy constructor. Is that intentional?
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
> BUG= Why no bug?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== base::Version move constructor BUG= ========== to ========== base::Version move constructor BUG=622707 ==========
On 2016/07/19 19:59:47, danakj wrote: > > BUG= > > Why no bug? I have added the bug number. For a more direct reason for this CL, please see issue https://codereview.chromium.org/2147693002/
Thanks for the context and linking a bug. Question remains about the copy constructor. On Tue, Jul 19, 2016 at 1:40 PM, <staraz@chromium.org> wrote: > On 2016/07/19 19:59:47, danakj wrote: > > > BUG= > > > > Why no bug? > > I have added the bug number. > For a more direct reason for this CL, please see issue > https://codereview.chromium.org/2147693002/ > > https://codereview.chromium.org/2163033002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2163033002/diff/1/base/version.cc File base/version.cc (right): https://codereview.chromium.org/2163033002/diff/1/base/version.cc#newcode97 base/version.cc:97: components_ = components; On 2016/07/19 19:56:46, Fady Samuel wrote: > Add to initializer list. Done. https://codereview.chromium.org/2163033002/diff/1/base/version.h File base/version.h (right): https://codereview.chromium.org/2163033002/diff/1/base/version.h#newcode35 base/version.h:35: explicit Version(std::vector<uint32_t>&& components); On 2016/07/19 19:59:27, danakj wrote: > this should come with an operator=. > > This also deletes the implicit copy constructor. Is that intentional? Done. A copy constructor is explicitly defined (and set to default) at line 26 and base/version.cc:83.
https://codereview.chromium.org/2163033002/diff/1/base/version.h File base/version.h (right): https://codereview.chromium.org/2163033002/diff/1/base/version.h#newcode35 base/version.h:35: explicit Version(std::vector<uint32_t>&& components); On 2016/07/19 20:46:13, StarAZ1 wrote: > On 2016/07/19 19:59:27, danakj wrote: > > this should come with an operator=. > > > > This also deletes the implicit copy constructor. Is that intentional? > > Done. > A copy constructor is explicitly defined (and set to default) at line 26 and > base/version.cc:83. Oh, there it is. Can you reorder this so that all the constructors are coming before the destructor? https://codereview.chromium.org/2163033002/diff/20001/base/version.cc File base/version.cc (right): https://codereview.chromium.org/2163033002/diff/20001/base/version.cc#newcode83 base/version.cc:83: Version::Version(const Version& other) = default; please order things in here to match the header when you fix ordering there https://codereview.chromium.org/2163033002/diff/20001/base/version.cc#newcode99 base/version.cc:99: Version& Version::operator=(Version&& other) { can you just use = default? https://codereview.chromium.org/2163033002/diff/20001/base/version.cc#newcode104 base/version.cc:104: Version& Version::operator=(const Version& other) { ditto, =default? https://codereview.chromium.org/2163033002/diff/20001/base/version.h File base/version.h (right): https://codereview.chromium.org/2163033002/diff/20001/base/version.h#newcode35 base/version.h:35: explicit Version(std::vector<uint32_t>&& components); This is not a valid constructor in the style guide. If you want to add a constructor for a vector of components, take it by value. But this isn't listed in the patch description either, why is it here? https://codereview.chromium.org/2163033002/diff/20001/base/version.h#newcode38 base/version.h:38: nit: you can use less whitespace and group the copy/move constructors, and the operators, together? also put the operator= in the same order as the constructors (copy first in this case)?
https://codereview.chromium.org/2163033002/diff/20001/base/version.h File base/version.h (right): https://codereview.chromium.org/2163033002/diff/20001/base/version.h#newcode35 base/version.h:35: explicit Version(std::vector<uint32_t>&& components); On 2016/07/19 20:50:07, danakj wrote: > This is not a valid constructor in the style guide. If you want to add a > constructor for a vector of components, take it by value. But this isn't listed > in the patch description either, why is it here? Sorry, I misguided Alex. I told him we want a move constructor for the components. I guess we have no way in the style guide from preventing call sites from copying the vector instead. I know this discussion is happening for Blink as well.
https://codereview.chromium.org/2163033002/diff/20001/base/version.h File base/version.h (right): https://codereview.chromium.org/2163033002/diff/20001/base/version.h#newcode35 base/version.h:35: explicit Version(std::vector<uint32_t>&& components); On 2016/07/19 21:05:59, Fady Samuel wrote: > On 2016/07/19 20:50:07, danakj wrote: > > This is not a valid constructor in the style guide. If you want to add a > > constructor for a vector of components, take it by value. But this isn't > listed > > in the patch description either, why is it here? > > Sorry, I misguided Alex. I told him we want a move constructor for the > components. I guess we have no way in the style guide from preventing call sites > from copying the vector instead. I know this discussion is happening for Blink > as well. Right, use move if you don't want to copy. So I'm not sure which one you actually want on this class here. Version to be moveable, or a constructor form integer components. But I'd prefer a move constructor. While the comopnents are exposed as a member, parsing is an implementation detail here and I wouldn't want other people trying to parse them.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== base::Version move constructor BUG=622707 ========== to ========== The new constructor is to be used by Version's mojom struct and its type mapping. BUG=622707 ==========
https://codereview.chromium.org/2163033002/diff/20001/base/version.cc File base/version.cc (right): https://codereview.chromium.org/2163033002/diff/20001/base/version.cc#newcode83 base/version.cc:83: Version::Version(const Version& other) = default; On 2016/07/19 20:50:07, danakj wrote: > please order things in here to match the header when you fix ordering there Done. https://codereview.chromium.org/2163033002/diff/20001/base/version.cc#newcode99 base/version.cc:99: Version& Version::operator=(Version&& other) { On 2016/07/19 20:50:07, danakj wrote: > can you just use = > default? Done. https://codereview.chromium.org/2163033002/diff/20001/base/version.cc#newcode104 base/version.cc:104: Version& Version::operator=(const Version& other) { On 2016/07/19 20:50:07, danakj wrote: > ditto, =default? Done. https://codereview.chromium.org/2163033002/diff/20001/base/version.h File base/version.h (right): https://codereview.chromium.org/2163033002/diff/20001/base/version.h#newcode35 base/version.h:35: explicit Version(std::vector<uint32_t>&& components); On 2016/07/19 21:18:32, danakj wrote: > On 2016/07/19 21:05:59, Fady Samuel wrote: > > On 2016/07/19 20:50:07, danakj wrote: > > > This is not a valid constructor in the style guide. If you want to add a > > > constructor for a vector of components, take it by value. But this isn't > > listed > > > in the patch description either, why is it here? > > > > Sorry, I misguided Alex. I told him we want a move constructor for the > > components. I guess we have no way in the style guide from preventing call > sites > > from copying the vector instead. I know this discussion is happening for Blink > > as well. > > Right, use move if you don't want to copy. So I'm not sure which one you > actually want on this class here. Version to be moveable, or a constructor form > integer components. But I'd prefer a move constructor. While the comopnents are > exposed as a member, parsing is an implementation detail here and I wouldn't > want other people trying to parse them. I changed the constructor to take the vector by value. The new constructor will only be used for the type mapping with rvalue vector. All components are still parsed here. https://codereview.chromium.org/2163033002/diff/20001/base/version.h#newcode38 base/version.h:38: On 2016/07/19 20:50:07, danakj wrote: > nit: you can use less whitespace and group the copy/move constructors, and the > operators, together? also put the operator= in the same order as the > constructors (copy first in this case)? I removed the new operator='s since they are no longer needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== The new constructor is to be used by Version's mojom struct and its type mapping. BUG=622707 ========== to ========== base::Version constructor from std::vector<uint32_t> The new constructor is to be used by Version's mojom struct and its type mapping. BUG=622707 ==========
LGTM with comment nits. Also the CL description won't include the subject. I copied the new subject line to the top of the description. https://codereview.chromium.org/2163033002/diff/40001/base/version.h File base/version.h (right): https://codereview.chromium.org/2163033002/diff/40001/base/version.h#newcode33 base/version.h:33: // Initializes from a vector of components, like {1, 2, 3, 4} Comments are sentences, including punctuation at the end. https://codereview.chromium.org/2163033002/diff/40001/base/version.h#newcode34 base/version.h:34: // This construct is used by Version's struct traits and should be called Can you remove this comment about rvalue vectors. People should always pass vectors by move if appropriate.
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2163033002/#ps60001 (title: "Changes based on nit comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== base::Version constructor from std::vector<uint32_t> The new constructor is to be used by Version's mojom struct and its type mapping. BUG=622707 ========== to ========== base::Version constructor from std::vector<uint32_t> The new constructor is to be used by Version's mojom struct and its type mapping. BUG=622707 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== base::Version constructor from std::vector<uint32_t> The new constructor is to be used by Version's mojom struct and its type mapping. BUG=622707 ========== to ========== base::Version constructor from std::vector<uint32_t> The new constructor is to be used by Version's mojom struct and its type mapping. BUG=622707 Committed: https://crrev.com/8fb38080e8833699973f510470fe2664e4fd3df6 Cr-Commit-Position: refs/heads/master@{#407529} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8fb38080e8833699973f510470fe2664e4fd3df6 Cr-Commit-Position: refs/heads/master@{#407529} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
