VariationsService now supports wildcard in min/max version

base/version.cc

Index: base/version.cc
===================================================================
--- base/version.cc	(revision 142948)
+++ base/version.cc	(working copy)
@@ -11,6 +11,34 @@
 #include "base/string_split.h"
 #include "base/string_util.h"
 
+namespace {
+
+// This function parses the |numbers| vector representing the different numbers
+// 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.
+bool ParseVersionNumbers(const std::vector<std::string>& numbers,

[Alexei Svitkine (slow), 2012/06/19 18:30:27]

Please document the return value.

[Mathieu, 2012/06/19 20:01:35]

Done.

+                         std::vector<uint16>& parsed) {
+  for (std::vector<std::string>::const_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;
+    parsed.push_back(static_cast<uint16>(num));
+  }
+  return true;
+}
+
+}  // namespace
+
 Version::Version() {
 }
 
@@ -23,21 +51,8 @@
   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));
-  }
+  if (!ParseVersionNumbers(numbers, parsed))
+    return;
   components_.swap(parsed);
 }
 
@@ -45,6 +60,23 @@
   return (!components_.empty());
 }
 
+// static
+bool Version::IsValidWildcard(const std::string& version_str) {
+  if (version_str.empty())
+    return false;
+
+  if (version_str.length() == 1 ||
+      version_str.compare(version_str.length() - 2, 2, ".*") != 0) {

[Alexei Svitkine (slow), 2012/06/19 18:30:27]

Use EndsWith() from base/string_util.h.

[Mathieu, 2012/06/19 20:01:35]

Done.

+    // Either a lone character or a full version number.
+    Version version(version_str);
+    return version.IsValid();
+  } else {

[Alexei Svitkine (slow), 2012/06/19 18:30:27]

Nit: Don't need an else if the previous block is an early return.

[Mathieu, 2012/06/19 20:01:35]

Done.

+    // Version strings length 2 or more, ending with ".*".
+    Version version(version_str.substr(0, version_str.size() - 2));
+    return version.IsValid();
+  }
+}
+
 bool Version::IsOlderThan(const std::string& version_str) const {
   Version proposed_ver(version_str);
   if (!proposed_ver.IsValid())
@@ -52,6 +84,39 @@
   return (CompareTo(proposed_ver) < 0);
 }
 
+int Version::CompareToWildcard(const std::string& version_str) const {
+  DCHECK(IsValid());
+  DCHECK(Version::IsValidWildcard(version_str));
+
+  // Default behavior if the string doesn't end with a wildcard.
+  if (version_str.compare(version_str.length() - 2, 2, ".*") != 0) {

[Alexei Svitkine (slow), 2012/06/19 18:30:27]

Use EndsWith() from base/string_util.h

[Mathieu, 2012/06/19 20:01:35]

Done.

+    Version version(version_str);
+    DCHECK(version.IsValid());
+    return CompareTo(version);
+  }
+
+  // Removing the * and returning immediately if the argument version is less

[Alexei Svitkine (slow), 2012/06/19 18:30:27]

Nit: Use a different verb tense, e.g.:

// Remove the "*" and return immediately if ...

[Mathieu, 2012/06/19 20:01:35]

Done.

+  // or equal (the * won't affect here).
+  Version other_version(version_str.substr(0, version_str.size() - 2));
+  int comparison = CompareTo(other_version);

[Alexei Svitkine (slow), 2012/06/19 18:30:27]

Is there a way to avoid having to do this (i.e. parsing the string twice - once
here and another time in ParseVersionNumbers)?

For example, can ParseVersionNumbers() return the number of components that
matched?

[Mathieu, 2012/06/19 20:01:35]

Done.

+  if (comparison == -1 || comparison == 0)
+    return comparison;
+
+  // Trying to catch the equality case 1.2 vs 1.*. In all other cases, it truly

[Alexei Svitkine (slow), 2012/06/19 18:30:27]

Nit: Reword comment. "Trying" doesn't sound very confident. Use a style that
describes what the code does.

I am also not sure what you're referring to when you say "case 1.2 vs, 1.*" - it
could mean any number of properties from that case (e.g. when it's the same
length or when the starts are the same and the difference is in suffix only -
vs. just having a different prefix.) Be more clear please.

[Mathieu, 2012/06/19 20:01:35]

Done.

+  // is a "greater than" case.
+  std::vector<std::string> other_numbers;
+  base::SplitString(version_str, '.', &other_numbers);
+  std::vector<uint16> parsed;
+  ParseVersionNumbers(other_numbers, parsed);  // Will not parse the *.

[Alexei Svitkine (slow), 2012/06/19 18:30:27]

Assign the result to a local "bool success" variable and DCHECK() on it on the
next line.

[Mathieu, 2012/06/19 20:01:35]

Done.

+
+  DCHECK_GT(parsed.size(), 0UL);
+  for (size_t i = 0; i < parsed.size(); ++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);