base::Version constructor from std::vector<uint32_t>

base/version.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_VERSION_H_
 #define BASE_VERSION_H_
 
 #include <stdint.h>
 
 #include <iosfwd>
@@ skipping 14 matching lines @@
 
   Version(const Version& other);
 
   ~Version();
 
   // Initializes from a decimal dotted version number, like "0.1.1".
   // Each component is limited to a uint16_t. Call IsValid() to learn
   // the outcome.
   explicit Version(const std::string& version_str);
 
+  explicit Version(std::vector<uint32_t>&& components);

[danakj, 2016/07/19 20:50:07]

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?

[Fady Samuel, 2016/07/19 21:05:59]

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.

[danakj, 2016/07/19 21:18:32]

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.

[Alex Z., 2016/07/22 13:28:03]

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.

+
+  Version& operator=(Version&& other);
+

[danakj, 2016/07/19 20:50:07]

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)?

[Alex Z., 2016/07/22 13:28:03]

I removed the new operator='s since they are no longer needed.

+  Version& operator=(const Version& other);
+
   // Returns true if the object contains a valid version number.
   bool IsValid() const;
 
   // Returns true if the version wildcard string is valid. The version wildcard
   // string may end with ".*" (e.g. 1.2.*, 1.*). Any other arrangement with "*"
   // is invalid (e.g. 1.*.3 or 1.2.3*). This functions defaults to standard
   // Version behavior (IsValid) if no wildcard is present.
   static bool IsValidWildcardString(const std::string& wildcard_string);
 
   // Returns -1, 0, 1 for <, ==, >.
@@ skipping 22 matching lines @@
 BASE_EXPORT bool operator>=(const Version& v1, const Version& v2);
 BASE_EXPORT std::ostream& operator<<(std::ostream& stream, const Version& v);
 
 }  // namespace base
 
 // TODO(xhwang) remove this when all users are updated to explicitly use the
 // namespace
 using base::Version;
 
 #endif  // BASE_VERSION_H_