VariationsService now supports wildcard in min/max version

base/version.cc

 // Copyright (c) 2011 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.
 
 #include "base/version.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
 
+namespace {
+
+// This function parses the |numbers| vector representing the different numbers

[Alexei Svitkine (slow), 2012/06/26 18:22:03]

Nit: Remove "This function" - just start the sentence with "Parses".

[Mathieu, 2012/06/27 15:40:06]

Done.

+// inside the version string and constructs a vector of valid integers. It stops
+// when it reaches an invalid item (including the wildcard character). |parsed|
+// is the resulting integer vector. Function returns true if all numbers were
+// parsed successfully, false otherwise.
+bool ParseVersionNumbers(const std::string& version_str,
+                         std::vector<uint16>& parsed) {

[Ilya Sherman, 2012/06/26 20:53:04]

nit: This should be passed as a pointer, rather than as a reference.  We only
pass const references into functions, so that it is easier at-a-glance to spot
which parameters can be updated as a side-effect of the function call.

[Mathieu, 2012/06/27 15:40:06]

Done.

+  std::vector<std::string> numbers;
+  base::SplitString(version_str, '.', &numbers);
+  if (numbers.empty())
+    return false;

[Ilya Sherman, 2012/06/26 20:53:04]

Optional nit: It might be helpful to add a blank line after this return stmt,
and perhaps to similarly space out some of the other blocks within this method
as well.

[Mathieu, 2012/06/27 15:40:06]

Done.

+  for (std::vector<std::string>::const_iterator i = numbers.begin();

[Ilya Sherman, 2012/06/26 20:53:04]

Optional nit: I generally prefer to use "it" rather than "i" for iterators,
unless there's a pre-existing convention in the code I'm editing to do
otherwise.  When reading code, I find "*i" or "i->second" a little surprising,
because I expect "i" to be an index.

[Mathieu, 2012/06/27 20:25:56]

Done.

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

[Ilya Sherman, 2012/06/26 20:53:04]

Optional nit: I assume that this does the right thing and upcasts max to an int,
rather than downcasting num to a uint16, but it might be nice to add a test to
verify that (and possible just to declare max as an int).

+      return false;
+    // This throws out things like +3, or 032.
+    if (base::IntToString(num) != *i)
+      return false;
+    parsed.push_back(static_cast<uint16>(num));
+  }
+  return true;
+}
+
+// Compares version components in |comp1| with components in |comp2|. Returns
+// -1, 0 or 1 if |comp1| is greater than, equal to, or less than |comp2|,
+// respectively.
+int CompareVersionComponents(const std::vector<uint16>& comp1,

[Ilya Sherman, 2012/06/26 20:53:04]

nit: Prefer to avoid abbreviations, so name this "components1" rather than
"comp1".

[Mathieu, 2012/06/27 15:40:06]

Done.

+                             const std::vector<uint16>& comp2) {
+  const size_t count = std::min(comp1.size(), comp2.size());
+  for (size_t i = 0; i < count; ++i) {
+    if (comp1[i] > comp2[i])
+      return 1;
+    if (comp1[i] < comp2[i])
+      return -1;
+  }

[Ilya Sherman, 2012/06/26 20:53:04]

nit: Perhaps the logic below this line should remain in Version::CompareTo(),
rather than appearing here and requiring the extra bit of logic at the end of
CompareToWildcardString() to essentially undo this?

+  if (comp1.size() > comp2.size()) {
+    for (size_t i = count; i < comp1.size(); ++i)
+      if (comp1[i] > 0)
+        return 1;
+  } else if (comp1.size() < comp2.size()) {
+    for (size_t i = count; i < comp2.size(); ++i)
+      if (comp2[i] > 0)
+        return -1;
+  }
+  return 0;
+}
+
+}  // namespace
+
 Version::Version() {
 }
 
 Version::~Version() {
 }
 
 Version::Version(const std::string& version_str) {
-  std::vector<std::string> numbers;
-  base::SplitString(version_str, '.', &numbers);
-  if (numbers.empty())
+  std::vector<uint16> parsed;
+  if (!ParseVersionNumbers(version_str, parsed))
     return;

[Ilya Sherman, 2012/06/26 20:53:04]

Optional nit: Add a blank line after this return stmt

[Mathieu, 2012/06/27 15:40:06]

Done.

-  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);
 }
 
 bool Version::IsValid() const {
   return (!components_.empty());
 }
 
+// static
+bool Version::IsValidWildcardString(const std::string& wildcard_string) {
+  if (wildcard_string.empty())

[Alexei Svitkine (slow), 2012/06/26 18:22:03]

Is this needed anymore or will this be covered by the !EndsWith() case?

[Mathieu, 2012/06/27 15:40:06]

Done.

+    return false;
+
+  if (!EndsWith(wildcard_string.c_str(), ".*", false)) {
+    // Either a lone character or a full version number.
+    Version version(wildcard_string);
+    return version.IsValid();

[Ilya Sherman, 2012/06/26 20:53:04]

Optional nit: These two lines are essentially repeated below.  Perhaps just trim
the string in the if-stmt, and then make the version.IsValid() call in just one
place?

[Mathieu, 2012/06/27 15:40:06]

Done.

+  }
+  // Version strings length 2 or more, ending with ".*".
+  Version version(wildcard_string.substr(0, wildcard_string.size() - 2));
+  return version.IsValid();
+}
+
 bool Version::IsOlderThan(const std::string& version_str) const {
   Version proposed_ver(version_str);
   if (!proposed_ver.IsValid())
     return false;
   return (CompareTo(proposed_ver) < 0);
 }
 
+int Version::CompareToWildcardString(const std::string& wildcard_string) const {
+  DCHECK(IsValid());
+  DCHECK(Version::IsValidWildcardString(wildcard_string));
+
+  // Default behavior if the string doesn't end with a wildcard.
+  if (!EndsWith(wildcard_string.c_str(), ".*", false)) {
+    Version version(wildcard_string);
+    DCHECK(version.IsValid());
+    return CompareTo(version);
+  }
+
+  std::vector<uint16> parsed;
+  const bool success = ParseVersionNumbers(
+      wildcard_string.substr(0, wildcard_string.length() - 2), parsed);
+  DCHECK(success);
+  const int comparison = CompareVersionComponents(components_, parsed);
+  // If the version is smaller than the wildcard version's |parsed| vector,
+  // then the wildcard has no effect (e.g. comparing 1.2.3 and 1.3.*) and the
+  // version is still smaller. Same logic for equality (e.g. comparing 1.2.2 to
+  // 1.2.2.* is 0 regardless of the wildcard).

[Ilya Sherman, 2012/06/26 20:53:04]

nit: I think |comparison| will also be 0 when comparing 1.2.0.0.0.0 to 1.2.*. 
If that's accurate, you might want to mention this case explicitly in the
comment.

[Mathieu, 2012/06/27 15:40:06]

Done.

+  if (comparison == -1 || comparison == 0)
+    return comparison;
+
+  // Catch the case where the digits of |parsed| are found in |components_|,
+  // which means that the two are equal since |parsed| has a trailing "*".
+  // (e.g. 1.2.3 vs. 1.2.* will return 0). All other cases return 1 since
+  // components is greater (e.g. 3.2.3 vs 1.*).
+  DCHECK_GT(parsed.size(), 0UL);
+  const size_t min_num_comp = std::min(components_.size(), parsed.size());
+  for (size_t i = 0; i < min_num_comp; ++i) {
+    if (components_[i] != parsed[i])
+      return 1;
+  }
+  return 0;
+}
+
 // TODO(cpu): remove this method.
 Version* Version::GetVersionFromString(const std::string& version_str) {
   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(IsValid());
   return new Version(*this);
 }
 
 bool Version::Equals(const Version& that) const {
   DCHECK(IsValid());
   DCHECK(that.IsValid());
   return (CompareTo(that) == 0);
 }
 
 int Version::CompareTo(const Version& other) const {
   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])
-      return 1;
-    if (components_[i] < other.components_[i])
-      return -1;
-  }
-  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)
-        return -1;
-  }
-  return 0;
+  return CompareVersionComponents(components_, other.components_);
 }
 
 const std::string Version::GetString() const {
   DCHECK(IsValid());
   std::string version_str;
   size_t count = components_.size();
   for (size_t i = 0; i < count - 1; ++i) {
     version_str.append(base::IntToString(components_[i]));
     version_str.append(".");
   }
   version_str.append(base::IntToString(components_[count - 1]));
   return version_str;
 }