Fixed bug where an empty version string is considered valid.

Made default constructor public, but DCHECK() on any use of a
default-constructed object.

BUG=none
TEST=unittests

Review URL: http://codereview.chromium.org/1364002

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

3 files changed, 53 insertions, 18 deletions

diff --git a/base/version.cc b/base/version.cc
index 83dda50..2271fcf 100644
--- a/base/version.cc
+++ b/base/version.cc
@@ -2,10 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <vector>
+#include "base/version.h"
 
+#include "base/logging.h"
 #include "base/string_util.h"
-#include "base/version.h"
 
 // static
 Version* Version::GetVersionFromString(const std::wstring& version_str) {
@@ -17,38 +17,46 @@ Version* Version::GetVersionFromString(const std::wstring& version_str) {
 // static
 Version* Version::GetVersionFromString(const std::string& version_str) {
   Version* vers = new Version();
-  if (vers->InitFromString(version_str))
+  if (vers->InitFromString(version_str)) {
+    DCHECK(vers->is_valid_);
     return vers;
+  }
   delete vers;
   return NULL;
 }
 
+Version::Version() : is_valid_(false) {}
+
 bool Version::Equals(const Version& that) const {
+  DCHECK(is_valid_);
+  DCHECK(that.is_valid_);
   return CompareTo(that) == 0;
 }
 
 int Version::CompareTo(const Version& other) const {
-  std::vector<uint16> other_components = other.components();
-  size_t count = std::min(components_.size(), other_components.size());
+  DCHECK(is_valid_);
+  DCHECK(other.is_valid_);
+  size_t count = std::min(components_.size(), other.components_.size());
   for (size_t i = 0; i < count; ++i) {
-    if (components_[i] > other_components[i])
+    if (components_[i] > other.components_[i])
       return 1;
-    if (components_[i] < other_components[i])
+    if (components_[i] < other.components_[i])
       return -1;
   }
-  if (components_.size() > other_components.size()) {
+  if (components_.size() > other.components_.size()) {
     for (size_t i = count; i < components_.size(); ++i)
       if (components_[i] > 0)
         return 1;
-  } else if (components_.size() < other_components.size()) {
-    for (size_t i = count; i < other_components.size(); ++i)
-      if (other_components[i] > 0)
+  } else if (components_.size() < other.components_.size()) {
+    for (size_t i = count; i < other.components_.size(); ++i)
+      if (other.components_[i] > 0)
         return -1;
   }
   return 0;
 }
 
 const std::string Version::GetString() const {
+  DCHECK(is_valid_);
   std::string version_str;
   int count = components_.size();
   for (int i = 0; i < count - 1; ++i) {
@@ -60,8 +68,11 @@ const std::string Version::GetString() const {
 }
 
 bool Version::InitFromString(const std::string& version_str) {
+  DCHECK(!is_valid_);
   std::vector<std::string> numbers;
   SplitString(version_str, '.', &numbers);
+  if (numbers.empty())
+    return false;
   for (std::vector<std::string>::iterator i = numbers.begin();
        i != numbers.end(); ++i) {
     int num;
@@ -78,5 +89,6 @@ bool Version::InitFromString(const std::string& version_str) {
     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 b2ad7f5..6b0680a 100644
--- a/base/version.h
+++ b/base/version.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"
 
 class Version {
  public:
@@ -18,6 +19,11 @@ class Version {
   static Version* GetVersionFromString(const std::wstring& version_str);
   static Version* GetVersionFromString(const std::string& version_str);
 
+  // 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.
+  Version();
+
   ~Version() {}
 
   bool Equals(const Version& other) const;
@@ -31,10 +37,14 @@ class Version {
   const std::vector<uint16>& components() const { return components_; }
 
  private:
-  Version() {}
   bool InitFromString(const std::string& version_str);
 
+  bool is_valid_;
   std::vector<uint16> components_;
+
+  FRIEND_TEST(VersionTest, DefaultConstructor);
+  FRIEND_TEST(VersionTest, GetVersionFromString);
+  FRIEND_TEST(VersionTest, Compare);
 };
 
 #endif  // BASE_VERSION_H_
diff --git a/base/version_unittest.cc b/base/version_unittest.cc
index d2cf98b..2e3c2ca 100644
--- a/base/version_unittest.cc
+++ b/base/version_unittest.cc
@@ -6,14 +6,27 @@
 #include "base/version.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace {
+class VersionTest : public testing::Test {
+};
 
-TEST(Version, GetVersionFromString) {
+TEST_F(VersionTest, DefaultConstructor) {
+  Version v;
+  EXPECT_FALSE(v.is_valid_);
+}
+
+TEST_F(VersionTest, GetVersionFromString) {
   static const struct version_string {
     const char* input;
     size_t parts;
     bool success;
   } cases[] = {
+    {"", 0, false},
+    {" ", 0, false},
+    {"\t", 0, false},
+    {"\n", 0, false},
+    {"  ", 0, false},
+    {".", 0, false},
+    {" . ", 0, false},
     {"0", 1, true},
     {"0.0", 2, true},
     {"65537.0", 0, false},
@@ -29,12 +42,14 @@ TEST(Version, GetVersionFromString) {
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); ++i) {
     scoped_ptr<Version> vers(Version::GetVersionFromString(cases[i].input));
     EXPECT_EQ(cases[i].success, vers.get() != NULL);
-    if (cases[i].success)
+    if (cases[i].success) {
+      EXPECT_TRUE(vers->is_valid_);
       EXPECT_EQ(cases[i].parts, vers->components().size());
+    }
   }
 }
 
-TEST(Version, Compare) {
+TEST_F(VersionTest, Compare) {
   static const struct version_compare {
     const char* lhs;
     const char* rhs;
@@ -58,5 +73,3 @@ TEST(Version, Compare) {
           cases[i].lhs << " ? " << cases[i].rhs;
   }
 }
-
-}