Clean up base/Version

It turns out base/Version is really a value object but probably because of some serious accident or by
the machinations of a super villain, forgot his identity and now it thinks is a reference object,
only creatable in the heap and that could only spawn offsprings via cloning.

But fear not 'cause I've seen Version true nature; At its core is just a good 'ol vector<uint16>, which has
very respectable value semantics. Also I have removed the is_valid_ parasite as much as I could.

The old interface (GetVersionFromString and Clone) is kept so existing callers would not need to be modified.

BUG=none
TEST=included
Review URL: http://codereview.chromium.org/7105008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@88143 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 70 insertions, 61 deletions

diff --git a/base/version.cc b/base/version.cc
index 571672c..74c570c 100644
--- a/base/version.cc
+++ b/base/version.cc
@@ -10,40 +10,63 @@
 #include "base/string_number_conversions.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
-#include "base/utf_string_conversions.h"
 
-Version::Version() : is_valid_(false) {}
+Version::Version() {
+}
+
+Version::Version(const std::string& version_str) {
+  std::vector<std::string> numbers;
+  base::SplitString(version_str, '.', &numbers);
+  if (numbers.empty())
+    return;
+  std::vector<uint16> parsed;
+  for (std::vector<std::string>::iterator i = numbers.begin();
+       i != numbers.end(); ++i) {
+    int num;
+    if (!base::StringToInt(*i, &num))
+      return;
+    if (num < 0)
+      return;
+    const uint16 max = 0xFFFF;
+    if (num > max)
+      return;
+    // This throws out things like +3, or 032.
+    if (base::IntToString(num) != *i)
+      return;
+    parsed.push_back(static_cast<uint16>(num));
+  }
+  components_.swap(parsed);
+}
 
-Version::~Version() {}
+bool Version::IsValid() const {
+  return (!components_.empty());
+}
 
-// static
+// TODO(cpu): remove this method.
 Version* Version::GetVersionFromString(const std::string& version_str) {
-  Version* vers = new Version();
-  if (vers->InitFromString(version_str)) {
-    DCHECK(vers->is_valid_);
+  Version* vers = new Version(version_str);
+  if (vers->IsValid()) {
     return vers;
   }
   delete vers;
   return NULL;
 }
 
+// TODO(cpu): remove this method.
 Version* Version::Clone() const {
-  DCHECK(is_valid_);
-  Version* copy = new Version();
-  copy->components_ = components_;
-  copy->is_valid_ = true;
-  return copy;
+  DCHECK(IsValid());
+  return new Version(*this);
 }
 
 bool Version::Equals(const Version& that) const {
-  DCHECK(is_valid_);
-  DCHECK(that.is_valid_);
-  return CompareTo(that) == 0;
+  DCHECK(IsValid());
+  DCHECK(that.IsValid());
+  return (CompareTo(that) == 0);
 }
 
 int Version::CompareTo(const Version& other) const {
-  DCHECK(is_valid_);
-  DCHECK(other.is_valid_);
+  DCHECK(IsValid());
+  DCHECK(other.IsValid());
   size_t count = std::min(components_.size(), other.components_.size());
   for (size_t i = 0; i < count; ++i) {
     if (components_[i] > other.components_[i])
@@ -64,7 +87,7 @@ int Version::CompareTo(const Version& other) const {
 }
 
 const std::string Version::GetString() const {
-  DCHECK(is_valid_);
+  DCHECK(IsValid());
   std::string version_str;
   size_t count = components_.size();
   for (size_t i = 0; i < count - 1; ++i) {
@@ -74,29 +97,3 @@ const std::string Version::GetString() const {
   version_str.append(base::IntToString(components_[count - 1]));
   return version_str;
 }
-
-bool Version::InitFromString(const std::string& version_str) {
-  DCHECK(!is_valid_);
-  std::vector<std::string> numbers;
-  base::SplitString(version_str, '.', &numbers);
-  if (numbers.empty())
-    return false;
-  for (std::vector<std::string>::iterator i = numbers.begin();
-       i != numbers.end(); ++i) {
-    int num;
-    if (!base::StringToInt(*i, &num))
-      return false;
-    if (num < 0)
-      return false;
-    const uint16 max = 0xFFFF;
-    if (num > max)
-      return false;
-    // This throws out things like +3, or 032.
-    if (base::IntToString(num) != *i)
-      return false;
-    uint16 component = static_cast<uint16>(num);
-    components_.push_back(component);
-  }
-  is_valid_ = true;
-  return true;
-}
diff --git a/base/version.h b/base/version.h
index d256d06..7dcefb3 100644
--- a/base/version.h
+++ b/base/version.h
@@ -15,22 +15,27 @@
 
 // Version represents a dotted version number, like "1.2.3.4", supporting
 // parsing and comparison.
-// Each component is limited to a uint16.
 class BASE_API Version {
  public:
-  // Exposed only so that a Version can be stored in STL containers;
-  // any call to the methods below on a default-constructed Version
-  // will DCHECK.
+  // The only thing you can legally do to a default constructed
+  // Version object is assign to it.
   Version();
 
-  ~Version();
+  // Initializes from a decimal dotted version number, like "0.1.1".
+  // Each component is limited to a uint16. Call IsValid() to learn
+  // the outcome.
+  explicit Version(const std::string& version_str);
 
-  // The version string must be made up of 1 or more uint16's separated
-  // by '.'. Returns NULL if string is not in this format.
+  // Returns true if the object contains a valid version number.
+  bool IsValid() const;
+
+  // Returns NULL if the string is not in the proper format.
   // Caller is responsible for freeing the Version object once done.
+  // DO NOT USE FOR NEWER CODE.
   static Version* GetVersionFromString(const std::string& version_str);
 
-  // Creates a copy of this version. Caller takes ownership.
+  // Creates a copy of this version object. Caller takes ownership.
+  // DO NOT USE FOR NEWER CODE.
   Version* Clone() const;
 
   bool Equals(const Version& other) const;
@@ -44,14 +49,7 @@ class BASE_API Version {
   const std::vector<uint16>& components() const { return components_; }
 
  private:
-  bool InitFromString(const std::string& version_str);
-
-  bool is_valid_;
   std::vector<uint16> components_;
-
-  FRIEND_TEST_ALL_PREFIXES(VersionTest, DefaultConstructor);
-  FRIEND_TEST_ALL_PREFIXES(VersionTest, GetVersionFromString);
-  FRIEND_TEST_ALL_PREFIXES(VersionTest, Compare);
 };
 
 #endif  // BASE_VERSION_H_
diff --git a/base/version_unittest.cc b/base/version_unittest.cc
index 486ef7d..a3b1ac6 100644
--- a/base/version_unittest.cc
+++ b/base/version_unittest.cc
@@ -11,7 +11,21 @@ class VersionTest : public testing::Test {
 
 TEST_F(VersionTest, DefaultConstructor) {
   Version v;
-  EXPECT_FALSE(v.is_valid_);
+  EXPECT_FALSE(v.IsValid());
+}
+
+TEST_F(VersionTest, ValueSemantics) {
+  Version v1("1.2.3.4");
+  EXPECT_TRUE(v1.IsValid());
+  Version v3;
+  EXPECT_FALSE(v3.IsValid());
+  {
+    Version v2(v1);
+    v3 = v2;
+    EXPECT_TRUE(v2.IsValid());
+    EXPECT_TRUE(v1.Equals(v2));
+  }
+  EXPECT_TRUE(v3.Equals(v1));
 }
 
 TEST_F(VersionTest, GetVersionFromString) {
@@ -43,7 +57,7 @@ TEST_F(VersionTest, GetVersionFromString) {
     scoped_ptr<Version> vers(Version::GetVersionFromString(cases[i].input));
     EXPECT_EQ(cases[i].success, vers.get() != NULL);
     if (cases[i].success) {
-      EXPECT_TRUE(vers->is_valid_);
+      EXPECT_TRUE(vers->IsValid());
       EXPECT_EQ(cases[i].parts, vers->components().size());
     }
   }