|
|
|
Created:
8 years, 6 months ago by mathp Modified:
8 years, 5 months ago CC:
chromium-reviews, erikwright (departed), MAD, jar (doing other things), brettw-cc_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionSupporting wildcards in max/min version specifications in VariationsService.
Adds a method CompareToWildcardString that will return -1/0/1, similar to
CompareTo, when a version is smaller, equal to or greater than a wildcard
string such as "1.2.*". Added a method IsValidWildcardString that validates
the format of a wildcard string, and slightly refactored the Version class
to avoid code duplication. For example, CompareToWildcardString and CompareTo
share the new method CompareVersionComponents.
BUG=127077
TEST=See tests for VariationsService, Version
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145468
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed initial comments #
Total comments: 14
Patch Set 3 : Second review addressed #Patch Set 4 : Small fix #Patch Set 5 : Added a check in VariationsService for validity of min/max versions. #Patch Set 6 : Log statements. #
Total comments: 24
Patch Set 7 : Renaming #Patch Set 8 : refactoring in version.cc #
Total comments: 10
Patch Set 9 : Fixed a typo #
Total comments: 5
Patch Set 10 : Addressed comments #
Total comments: 32
Patch Set 11 : Added test cases, addressed comments #
Total comments: 14
Patch Set 12 : Round of fixes #
Total comments: 6
Patch Set 13 : minor fixes #
Total comments: 2
Patch Set 14 : addressed brettw's comment #
Messages
Total messages: 28 (0 generated)
Here are some initial comments - mostly focused on code style. (By the way, you didn't hit "Publish" in codereview - so I didn't get this in my email - merely noticed it when I went to codereview.chromium.org). http://codereview.chromium.org/10576003/diff/1/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode14 base/version.cc:14: namespace { Please add an empty line between the start of the namespace and the function declaration and another empty line after the function and the end of the namespace. Add a short comment before the function start explaining what it does and its parameters. http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode16 base/version.cc:16: std::vector<uint16>& parsed) { Align second parameter. http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode34 base/version.cc:34: } The namespace should end with a comment, like so: "} // namespace" (Note the two spaces between the } and the start of the comment.) http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode57 base/version.cc:57: bool Version::IsValidWildcard(const std::string& version_str) { For static methods, the convention is to put a comment "// static" right above the method start. http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode59 base/version.cc:59: Version version(version_str); Indent this better. http://codereview.chromium.org/10576003/diff/1/base/version_unittest.cc File base/version_unittest.cc (right): http://codereview.chromium.org/10576003/diff/1/base/version_unittest.cc#newco... base/version_unittest.cc:107: scoped_ptr<Version> lhs(Version::GetVersionFromString(cases[i].lhs)); Don't use GetVersionFromString() - it has a comment that says "DO NOT USE FOR NEWER CODE." Instead do: Version version(cases[i].lhs);
http://codereview.chromium.org/10576003/diff/1/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode14 base/version.cc:14: namespace { On 2012/06/18 22:03:44, Alexei Svitkine wrote: > Please add an empty line between the start of the namespace and the function > declaration and another empty line after the function and the end of the > namespace. > > Add a short comment before the function start explaining what it does and its > parameters. Done. http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode16 base/version.cc:16: std::vector<uint16>& parsed) { On 2012/06/18 22:03:44, Alexei Svitkine wrote: > Align second parameter. Done. http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode34 base/version.cc:34: } On 2012/06/18 22:03:44, Alexei Svitkine wrote: > The namespace should end with a comment, like so: > > "} // namespace" > > (Note the two spaces between the } and the start of the comment.) Done. http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode57 base/version.cc:57: bool Version::IsValidWildcard(const std::string& version_str) { On 2012/06/18 22:03:44, Alexei Svitkine wrote: > For static methods, the convention is to put a comment "// static" right above > the method start. Done. http://codereview.chromium.org/10576003/diff/1/base/version.cc#newcode59 base/version.cc:59: Version version(version_str); On 2012/06/18 22:03:44, Alexei Svitkine wrote: > Indent this better. Done. http://codereview.chromium.org/10576003/diff/1/base/version_unittest.cc File base/version_unittest.cc (right): http://codereview.chromium.org/10576003/diff/1/base/version_unittest.cc#newco... base/version_unittest.cc:107: scoped_ptr<Version> lhs(Version::GetVersionFromString(cases[i].lhs)); On 2012/06/18 22:03:44, Alexei Svitkine wrote: > Don't use GetVersionFromString() - it has a comment that says "DO NOT USE FOR > NEWER CODE." > > Instead do: > > Version version(cases[i].lhs); Done.
More comments for you! http://codereview.chromium.org/10576003/diff/6002/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/6002/base/version.cc#newcode65 base/version.cc:65: if (version_str.at(version_str.length() - 1) != '*') { This will crash if version_str is empty. http://codereview.chromium.org/10576003/diff/6002/base/version.h File base/version.h (right): http://codereview.chromium.org/10576003/diff/6002/base/version.h#newcode34 base/version.h:34: // can end with a wildcard character. Please reword this. "In this version," is very confusing here. There is no need to refer to the current state of the code as "in this version" - just state exactly what it does and what is valid and invalid. http://codereview.chromium.org/10576003/diff/6002/base/version.h#newcode59 base/version.h:59: // not end in a wildcard character "*". Please document the requirement that IsValidWildcard(version_str) must be true before calling this function. Also, suggest renaming names and variables throughout to refer to "wildcard_string", e.g.: CompareToWildcardString(const std::string& wildcard_string) and IsValidWildcardString(const std::string& wildcard_string) and elsewhere. http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc File base/version_unittest.cc (right): http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc#ne... base/version_unittest.cc:108: EXPECT_EQ(version.CompareToWildcard(cases[i].rhs), cases[i].expected) << Nit: Suggest extracting the result of version.CompareToWildcard(cases[i].rhs) to a local var so that the EXPECT_EQ line can take up just one line. http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc#ne... base/version_unittest.cc:118: {"1.0", true}, Please add a test for empty string. http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc#ne... base/version_unittest.cc:124: {"*.2", false}, How about a case like: "1.2.3*"? This should be invalid, because it's ambiguous (i.e. 1.2.35 vs. 1.2.3). Please also expand the comment for Version::IsValidWildcard() to be very clear about this. http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc#ne... base/version_unittest.cc:127: EXPECT_EQ(Version::IsValidWildcard(cases[i].version), cases[i].expected) << Nit: Suggest extracting the result of Version::IsValidWildcard(cases[i].version) to a local var so that the EXPECT_EQ line can take up just one line.
http://codereview.chromium.org/10576003/diff/6002/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/6002/base/version.cc#newcode65 base/version.cc:65: if (version_str.at(version_str.length() - 1) != '*') { On 2012/06/19 14:53:17, Alexei Svitkine wrote: > This will crash if version_str is empty. Done. Added a check for empty string. http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc File base/version_unittest.cc (right): http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc#ne... base/version_unittest.cc:108: EXPECT_EQ(version.CompareToWildcard(cases[i].rhs), cases[i].expected) << On 2012/06/19 14:53:17, Alexei Svitkine wrote: > Nit: Suggest extracting the result of version.CompareToWildcard(cases[i].rhs) to > a local var so that the EXPECT_EQ line can take up just one line. Done. http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc#ne... base/version_unittest.cc:118: {"1.0", true}, On 2012/06/19 14:53:17, Alexei Svitkine wrote: > Please add a test for empty string. Done. http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc#ne... base/version_unittest.cc:124: {"*.2", false}, On 2012/06/19 14:53:17, Alexei Svitkine wrote: > How about a case like: "1.2.3*"? > > This should be invalid, because it's ambiguous (i.e. 1.2.35 vs. 1.2.3). > > Please also expand the comment for Version::IsValidWildcard() to be very clear > about this. Done. Good point. http://codereview.chromium.org/10576003/diff/6002/base/version_unittest.cc#ne... base/version_unittest.cc:127: EXPECT_EQ(Version::IsValidWildcard(cases[i].version), cases[i].expected) << On 2012/06/19 14:53:17, Alexei Svitkine wrote: > Nit: Suggest extracting the result of Version::IsValidWildcard(cases[i].version) > to a local var so that the EXPECT_EQ line can take up just one line. Done. Can't really make it fit...
I think you missed this part of my comments before: > Also, suggest renaming names and variables throughout to refer to > "wildcard_string", e.g.: > > CompareToWildcardString(const std::string& wildcard_string) > > and > > IsValidWildcardString(const std::string& wildcard_string) and elsewhere.
http://codereview.chromium.org/10576003/diff/6002/base/version.h File base/version.h (right): http://codereview.chromium.org/10576003/diff/6002/base/version.h#newcode34 base/version.h:34: // can end with a wildcard character. On 2012/06/19 14:53:17, Alexei Svitkine wrote: > Please reword this. "In this version," is very confusing here. There is no need > to refer to the current state of the code as "in this version" - just state > exactly what it does and what is valid and invalid. Done. http://codereview.chromium.org/10576003/diff/6002/base/version.h#newcode59 base/version.h:59: // not end in a wildcard character "*". On 2012/06/19 14:53:17, Alexei Svitkine wrote: > Please document the requirement that IsValidWildcard(version_str) must be true > before calling this function. > > Also, suggest renaming names and variables throughout to refer to > "wildcard_string", e.g.: > > CompareToWildcardString(const std::string& wildcard_string) > > and > > IsValidWildcardString(const std::string& wildcard_string) and elsewhere. Done.
Looking much better. Here are some more meatier comments. http://codereview.chromium.org/10576003/diff/10005/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode20 base/version.cc:20: bool ParseVersionNumbers(const std::vector<std::string>& numbers, Please document the return value. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode50 base/version.cc:50: base::SplitString(version_str, '.', &numbers); How about folding the SplitString() code into |ParseVersionNumbers()|? http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode69 base/version.cc:69: version_str.compare(version_str.length() - 2, 2, ".*") != 0) { Use EndsWith() from base/string_util.h. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode73 base/version.cc:73: } else { Nit: Don't need an else if the previous block is an early return. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode92 base/version.cc:92: if (version_str.compare(version_str.length() - 2, 2, ".*") != 0) { Use EndsWith() from base/string_util.h http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode98 base/version.cc:98: // Removing the * and returning immediately if the argument version is less Nit: Use a different verb tense, e.g.: // Remove the "*" and return immediately if ... http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode101 base/version.cc:101: int comparison = CompareTo(other_version); 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? http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode105 base/version.cc:105: // Trying to catch the equality case 1.2 vs 1.*. In all other cases, it truly 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. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode110 base/version.cc:110: ParseVersionNumbers(other_numbers, parsed); // Will not parse the *. Assign the result to a local "bool success" variable and DCHECK() on it on the next line. http://codereview.chromium.org/10576003/diff/10005/base/version.h File base/version.h (right): http://codereview.chromium.org/10576003/diff/10005/base/version.h#newcode33 base/version.h:33: // Returns true if the version string is valid. In this version, the string You still have the "In this version" wording which I mentioned I didn't like in my previous comment. Also, be more precise "e.g. the string may be a valid version string or a string ending with ".*" wildcard." http://codereview.chromium.org/10576003/diff/10005/base/version_unittest.cc File base/version_unittest.cc (right): http://codereview.chromium.org/10576003/diff/10005/base/version_unittest.cc#n... base/version_unittest.cc:108: int result = version.CompareToWildcard(cases[i].rhs); Nit: Make version and result const. http://codereview.chromium.org/10576003/diff/10005/base/version_unittest.cc#n... base/version_unittest.cc:132: cases[i].version << " failed -> " << cases[i].expected; This should be indented 4 more than EXPECT_EQ().
http://codereview.chromium.org/10576003/diff/10005/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode20 base/version.cc:20: bool ParseVersionNumbers(const std::vector<std::string>& numbers, On 2012/06/19 18:30:27, Alexei Svitkine wrote: > Please document the return value. Done. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode50 base/version.cc:50: base::SplitString(version_str, '.', &numbers); On 2012/06/19 18:30:27, Alexei Svitkine wrote: > How about folding the SplitString() code into |ParseVersionNumbers()|? Done. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode69 base/version.cc:69: version_str.compare(version_str.length() - 2, 2, ".*") != 0) { On 2012/06/19 18:30:27, Alexei Svitkine wrote: > Use EndsWith() from base/string_util.h. Done. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode73 base/version.cc:73: } else { On 2012/06/19 18:30:27, Alexei Svitkine wrote: > Nit: Don't need an else if the previous block is an early return. Done. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode92 base/version.cc:92: if (version_str.compare(version_str.length() - 2, 2, ".*") != 0) { On 2012/06/19 18:30:27, Alexei Svitkine wrote: > Use EndsWith() from base/string_util.h Done. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode98 base/version.cc:98: // Removing the * and returning immediately if the argument version is less On 2012/06/19 18:30:27, Alexei Svitkine wrote: > Nit: Use a different verb tense, e.g.: > > // Remove the "*" and return immediately if ... Done. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode101 base/version.cc:101: int comparison = CompareTo(other_version); On 2012/06/19 18:30:27, Alexei Svitkine wrote: > 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? Done. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode105 base/version.cc:105: // Trying to catch the equality case 1.2 vs 1.*. In all other cases, it truly On 2012/06/19 18:30:27, Alexei Svitkine wrote: > 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. > Done. http://codereview.chromium.org/10576003/diff/10005/base/version.cc#newcode110 base/version.cc:110: ParseVersionNumbers(other_numbers, parsed); // Will not parse the *. On 2012/06/19 18:30:27, Alexei Svitkine wrote: > Assign the result to a local "bool success" variable and DCHECK() on it on the > next line. Done. http://codereview.chromium.org/10576003/diff/10005/base/version.h File base/version.h (right): http://codereview.chromium.org/10576003/diff/10005/base/version.h#newcode33 base/version.h:33: // Returns true if the version string is valid. In this version, the string On 2012/06/19 18:30:27, Alexei Svitkine wrote: > You still have the "In this version" wording which I mentioned I didn't like in > my previous comment. Also, be more precise "e.g. the string may be a valid > version string or a string ending with ".*" wildcard." Done. Sorry the updated version did not make it here. http://codereview.chromium.org/10576003/diff/10005/base/version_unittest.cc File base/version_unittest.cc (right): http://codereview.chromium.org/10576003/diff/10005/base/version_unittest.cc#n... base/version_unittest.cc:108: int result = version.CompareToWildcard(cases[i].rhs); On 2012/06/19 18:30:27, Alexei Svitkine wrote: > Nit: Make version and result const. Done. http://codereview.chromium.org/10576003/diff/10005/base/version_unittest.cc#n... base/version_unittest.cc:132: cases[i].version << " failed -> " << cases[i].expected; On 2012/06/19 18:30:27, Alexei Svitkine wrote: > This should be indented 4 more than EXPECT_EQ(). Done.
More comments your way! Also, please run some try jobs so that we can see this is green on the bots. http://codereview.chromium.org/10576003/diff/16001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode46 base/version.cc:46: // -1, 0 or 1 if comp2 is less than, equal to, or greater than comp1, Nit: Put |'s around |comp2| and |comp1| on this line. Also, please make this sentence mention comp1 first (in the same order as the parameters, i.e.: // -1, 0 or 1 if |comp1| is greater than, equal to, or less than |comp2|, http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode50 base/version.cc:50: size_t count = std::min(comp1.size(), comp2.size()); Nit: Make |count| const. http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode93 base/version.cc:93: if (wildcard_string.length() == 1 || You don't need the length == 1 check because EndsWith() will be false in that case anyway. http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode123 base/version.cc:123: bool success = ParseVersionNumbers( Nit: Make |success| const. http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode126 base/version.cc:126: int comparison = CompareVersionComponents(components_, parsed); Nit: Make |comparison| const. http://codereview.chromium.org/10576003/diff/22001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/22001/base/version.cc#newcode127 base/version.cc:127: if (comparison == -1 || comparison == 0) Please add a short comment explaining why it makes sense to return under these two circumstances with examples. (It was not immediately obvious to me until I worked it out on paper.) http://codereview.chromium.org/10576003/diff/22001/base/version.cc#newcode135 base/version.cc:135: for (size_t i = 0; i < parsed.size(); ++i) { Can components_.size() > parsed.size() be the case here? For example, comparing version "1.4" to wildcard string "1.3.0.*"? If so, please fix and add a test case. If not, please add a comment why that is the case.
http://codereview.chromium.org/10576003/diff/22001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/22001/base/version.cc#newcode135 base/version.cc:135: for (size_t i = 0; i < parsed.size(); ++i) { > Can components_.size() > parsed.size() be the case here? This meant to ask about "components_.size() < parsed.size()".
http://codereview.chromium.org/10576003/diff/16001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode46 base/version.cc:46: // -1, 0 or 1 if comp2 is less than, equal to, or greater than comp1, On 2012/06/21 21:00:53, Alexei Svitkine wrote: > Nit: Put |'s around |comp2| and |comp1| on this line. Also, please make this > sentence mention comp1 first (in the same order as the parameters, i.e.: > > // -1, 0 or 1 if |comp1| is greater than, equal to, or less than |comp2|, Done. http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode50 base/version.cc:50: size_t count = std::min(comp1.size(), comp2.size()); On 2012/06/21 21:00:53, Alexei Svitkine wrote: > Nit: Make |count| const. Done. http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode93 base/version.cc:93: if (wildcard_string.length() == 1 || On 2012/06/21 21:00:53, Alexei Svitkine wrote: > You don't need the length == 1 check because EndsWith() will be false in that > case anyway. Done. http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode123 base/version.cc:123: bool success = ParseVersionNumbers( On 2012/06/21 21:00:53, Alexei Svitkine wrote: > Nit: Make |success| const. Done. http://codereview.chromium.org/10576003/diff/16001/base/version.cc#newcode126 base/version.cc:126: int comparison = CompareVersionComponents(components_, parsed); On 2012/06/21 21:00:53, Alexei Svitkine wrote: > Nit: Make |comparison| const. Done. http://codereview.chromium.org/10576003/diff/22001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/22001/base/version.cc#newcode127 base/version.cc:127: if (comparison == -1 || comparison == 0) On 2012/06/21 21:00:53, Alexei Svitkine wrote: > Please add a short comment explaining why it makes sense to return under these > two circumstances with examples. (It was not immediately obvious to me until I > worked it out on paper.) Done. http://codereview.chromium.org/10576003/diff/22001/base/version.cc#newcode135 base/version.cc:135: for (size_t i = 0; i < parsed.size(); ++i) { On 2012/06/21 21:00:53, Alexei Svitkine wrote: > Can components_.size() > parsed.size() be the case here? > > For example, comparing version "1.4" to wildcard string "1.3.0.*"? > > If so, please fix and add a test case. If not, please add a comment why that is > the case. Done.
One final round of comments / nits from me. http://codereview.chromium.org/10576003/diff/40001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode16 base/version.cc:16: // This function parses the |numbers| vector representing the different numbers Nit: Remove "This function" - just start the sentence with "Parses". http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode90 base/version.cc:90: if (wildcard_string.empty()) Is this needed anymore or will this be covered by the !EndsWith() case? http://codereview.chromium.org/10576003/diff/40001/base/version.h File base/version.h (right): http://codereview.chromium.org/10576003/diff/40001/base/version.h#newcode33 base/version.h:33: // Returns true if the version string is valid. The version string Nit: "version string" -> "version wildcard string" (2x) http://codereview.chromium.org/10576003/diff/40001/base/version.h#newcode35 base/version.h:35: // be invalid (e.g. 1.*.3 or 1.2.3*). This functions defaults to standard Nit: Rephrase "will be" to "is". http://codereview.chromium.org/10576003/diff/40001/base/version_unittest.cc File base/version_unittest.cc (right): http://codereview.chromium.org/10576003/diff/40001/base/version_unittest.cc#n... base/version_unittest.cc:105: {"1.4", "1.3.0.*", 1}, Can you also add these testcases: 1.3.9 to 1.3.* 1.4.1 to 1.3.* 1.3 to 1.4.5.* 1.5 to 1.4.5.* http://codereview.chromium.org/10576003/diff/40001/chrome/browser/metrics/var... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10576003/diff/40001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:332: DVLOG(1) << study.filter().min_version() << " invalid min version."; Please log the study.name() like the other cases. http://codereview.chromium.org/10576003/diff/40001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:337: DVLOG(1) << study.filter().max_version() << " invalid max version."; Please log the study.name() like the other cases. http://codereview.chromium.org/10576003/diff/40001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:351: Nit: Remove unnecessary newline change here.
Also, please update the CL description for the new method name. Additionally, consider being a little more precise in the description (mention which class you're adding the method to and also list the other changes in this CL such as the validation, etc.)
Out of curiosity: Did you consider extending the semantics of the Version constructor to support wildcards, rather than adding the pair of wildcard methods to parallel the existing methods? http://codereview.chromium.org/10576003/diff/40001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode22 base/version.cc:22: std::vector<uint16>& parsed) { 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. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode26 base/version.cc:26: return false; 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. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode27 base/version.cc:27: for (std::vector<std::string>::const_iterator i = numbers.begin(); 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. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode35 base/version.cc:35: if (num > max) 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). http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode48 base/version.cc:48: int CompareVersionComponents(const std::vector<uint16>& comp1, nit: Prefer to avoid abbreviations, so name this "components1" rather than "comp1". http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode56 base/version.cc:56: } 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? http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode80 base/version.cc:80: return; Optional nit: Add a blank line after this return stmt http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode96 base/version.cc:96: return version.IsValid(); 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? http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode129 base/version.cc:129: // 1.2.2.* is 0 regardless of the wildcard). 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.
On Tue, Jun 26, 2012 at 4:53 PM, <isherman@chromium.org> wrote: > Out of curiosity: Did you consider extending the semantics of the Version > constructor to support wildcards, rather than adding the pair of wildcard > methods to parallel the existing methods? I advised against that approach since that would give confusing semantics to Version objects - right now they just represent a single version number, which is clear and simple. In the other case, it would introduce confusion as Version would now either be a real version or something that is not a version but that can be used to match against versions - this is undesirable for the same reason we have separate classes base::Time and base::TimeDelta - instead of a single base::Time that can hold both. -Alexei > http://codereview.chromium.**org/10576003/diff/40001/base/**version.cc<http:/... > File base/version.cc (right): > > http://codereview.chromium.**org/10576003/diff/40001/base/** > version.cc#newcode22<http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode22> > base/version.cc:22: std::vector<uint16>& parsed) { > 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. > Woops, I totally missed that - yes please pass |parsed| by pointer.
On 2012/06/26 21:00:19, Alexei Svitkine wrote: > On Tue, Jun 26, 2012 at 4:53 PM, <mailto:isherman@chromium.org> wrote: > > > Out of curiosity: Did you consider extending the semantics of the Version > > constructor to support wildcards, rather than adding the pair of wildcard > > methods to parallel the existing methods? > > > I advised against that approach since that would give confusing semantics > to Version objects - right now they just represent a single version number, > which is clear and simple. > > In the other case, it would introduce confusion as Version would now either > be a real version or something that is not a version but that can be used > to match against versions - this is undesirable for the same reason we have > separate classes base::Time and base::TimeDelta - instead of a single > base::Time that can hold both. Fair enough :)
http://codereview.chromium.org/10576003/diff/40001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode16 base/version.cc:16: // This function parses the |numbers| vector representing the different numbers On 2012/06/26 18:22:03, Alexei Svitkine wrote: > Nit: Remove "This function" - just start the sentence with "Parses". Done. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode22 base/version.cc:22: std::vector<uint16>& parsed) { On 2012/06/26 20:53:04, Ilya Sherman wrote: > 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. Done. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode26 base/version.cc:26: return false; On 2012/06/26 20:53:04, Ilya Sherman wrote: > 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. Done. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode48 base/version.cc:48: int CompareVersionComponents(const std::vector<uint16>& comp1, On 2012/06/26 20:53:04, Ilya Sherman wrote: > nit: Prefer to avoid abbreviations, so name this "components1" rather than > "comp1". Done. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode80 base/version.cc:80: return; On 2012/06/26 20:53:04, Ilya Sherman wrote: > Optional nit: Add a blank line after this return stmt Done. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode90 base/version.cc:90: if (wildcard_string.empty()) On 2012/06/26 18:22:03, Alexei Svitkine wrote: > Is this needed anymore or will this be covered by the !EndsWith() case? Done. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode96 base/version.cc:96: return version.IsValid(); On 2012/06/26 20:53:04, Ilya Sherman wrote: > 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? Done. http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode129 base/version.cc:129: // 1.2.2.* is 0 regardless of the wildcard). On 2012/06/26 20:53:04, Ilya Sherman wrote: > 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. Done. http://codereview.chromium.org/10576003/diff/40001/base/version.h File base/version.h (right): http://codereview.chromium.org/10576003/diff/40001/base/version.h#newcode33 base/version.h:33: // Returns true if the version string is valid. The version string On 2012/06/26 18:22:03, Alexei Svitkine wrote: > Nit: "version string" -> "version wildcard string" (2x) Done. http://codereview.chromium.org/10576003/diff/40001/base/version.h#newcode35 base/version.h:35: // be invalid (e.g. 1.*.3 or 1.2.3*). This functions defaults to standard On 2012/06/26 18:22:03, Alexei Svitkine wrote: > Nit: Rephrase "will be" to "is". Done. http://codereview.chromium.org/10576003/diff/40001/base/version_unittest.cc File base/version_unittest.cc (right): http://codereview.chromium.org/10576003/diff/40001/base/version_unittest.cc#n... base/version_unittest.cc:105: {"1.4", "1.3.0.*", 1}, On 2012/06/26 18:22:03, Alexei Svitkine wrote: > Can you also add these testcases: > > 1.3.9 to 1.3.* > 1.4.1 to 1.3.* > 1.3 to 1.4.5.* > 1.5 to 1.4.5.* Done. http://codereview.chromium.org/10576003/diff/40001/chrome/browser/metrics/var... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10576003/diff/40001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:332: DVLOG(1) << study.filter().min_version() << " invalid min version."; On 2012/06/26 18:22:03, Alexei Svitkine wrote: > Please log the study.name() like the other cases. Done. http://codereview.chromium.org/10576003/diff/40001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:337: DVLOG(1) << study.filter().max_version() << " invalid max version."; On 2012/06/26 18:22:03, Alexei Svitkine wrote: > Please log the study.name() like the other cases. Done. http://codereview.chromium.org/10576003/diff/40001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:351: On 2012/06/26 18:22:03, Alexei Svitkine wrote: > Nit: Remove unnecessary newline change here. Done.
http://codereview.chromium.org/10576003/diff/50001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/50001/base/version.cc#newcode51 base/version.cc:51: // |components2|. Returns -1, 0 or 1 if |comp1| is greater than, equal to, or This line still says "comp1". http://codereview.chromium.org/10576003/diff/50001/base/version.cc#newcode99 base/version.cc:99: wildcard_string.substr(0, wildcard_string.size() - 2); I don't like this flow since this now invokes the default constructor and then the copy constructor... Can you do this differently? http://codereview.chromium.org/10576003/diff/50001/base/version.h File base/version.h (right): http://codereview.chromium.org/10576003/diff/50001/base/version.h#newcode33 base/version.h:33: // Returns true if the version string is valid. The version wildcard string First sentence still says "version string". http://codereview.chromium.org/10576003/diff/50001/chrome/browser/metrics/var... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10576003/diff/50001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:332: DVLOG(1) << study.name() << " has invalid min version: " << Nit: Wrap this like so: DVLOG(1) << study.name() << " has invalid min version: " << study.filter().min_version(); http://codereview.chromium.org/10576003/diff/50001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:338: DVLOG(1) << study.name() << " has invalid max version: " << Nit: Wrap this differently too.
http://codereview.chromium.org/10576003/diff/50001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/50001/base/version.cc#newcode22 base/version.cc:22: std::vector<uint16> *parsed) { nit: " *parsed" -> "* parsed" http://codereview.chromium.org/10576003/diff/50001/base/version.cc#newcode100 base/version.cc:100: version = Version(trimmed_wc_string); nit: Please avoid abbreviations like "wc". In this case, "trimmed_string" is probably sufficient in context.
http://codereview.chromium.org/10576003/diff/40001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/40001/base/version.cc#newcode27 base/version.cc:27: for (std::vector<std::string>::const_iterator i = numbers.begin(); On 2012/06/26 20:53:04, Ilya Sherman wrote: > 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. Done. http://codereview.chromium.org/10576003/diff/50001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/50001/base/version.cc#newcode22 base/version.cc:22: std::vector<uint16> *parsed) { On 2012/06/27 17:16:42, Ilya Sherman wrote: > nit: " *parsed" -> "* parsed" Done. http://codereview.chromium.org/10576003/diff/50001/base/version.cc#newcode51 base/version.cc:51: // |components2|. Returns -1, 0 or 1 if |comp1| is greater than, equal to, or On 2012/06/27 15:47:59, Alexei Svitkine wrote: > This line still says "comp1". Done. Sorry. http://codereview.chromium.org/10576003/diff/50001/base/version.cc#newcode99 base/version.cc:99: wildcard_string.substr(0, wildcard_string.size() - 2); On 2012/06/27 15:47:59, Alexei Svitkine wrote: > I don't like this flow since this now invokes the default constructor and then > the copy constructor... Can you do this differently? Done. http://codereview.chromium.org/10576003/diff/50001/base/version.cc#newcode100 base/version.cc:100: version = Version(trimmed_wc_string); On 2012/06/27 17:16:42, Ilya Sherman wrote: > nit: Please avoid abbreviations like "wc". In this case, "trimmed_string" is > probably sufficient in context. Done. http://codereview.chromium.org/10576003/diff/50001/base/version.h File base/version.h (right): http://codereview.chromium.org/10576003/diff/50001/base/version.h#newcode33 base/version.h:33: // Returns true if the version string is valid. The version wildcard string On 2012/06/27 15:47:59, Alexei Svitkine wrote: > First sentence still says "version string". Done. http://codereview.chromium.org/10576003/diff/50001/chrome/browser/metrics/var... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10576003/diff/50001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:332: DVLOG(1) << study.name() << " has invalid min version: " << On 2012/06/27 15:47:59, Alexei Svitkine wrote: > Nit: Wrap this like so: > > DVLOG(1) << study.name() << " has invalid min version: " > << study.filter().min_version(); Done. http://codereview.chromium.org/10576003/diff/50001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:338: DVLOG(1) << study.name() << " has invalid max version: " << On 2012/06/27 15:47:59, Alexei Svitkine wrote: > Nit: Wrap this differently too. Done.
LGTM with a few more nits. Adding brettw@ for base/ approval. http://codereview.chromium.org/10576003/diff/58001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/58001/base/version.cc#newcode97 base/version.cc:97: if (EndsWith(wildcard_string.c_str(), ".*", false)) { Nit: 1 line if statements don't need braces. http://codereview.chromium.org/10576003/diff/58001/chrome/browser/metrics/var... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10576003/diff/58001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:333: << study.filter().min_version(); Nit: Align with << on the line above. http://codereview.chromium.org/10576003/diff/58001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:339: << study.filter().max_version(); Nit: Align with << on the line above.
http://codereview.chromium.org/10576003/diff/58001/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/58001/base/version.cc#newcode97 base/version.cc:97: if (EndsWith(wildcard_string.c_str(), ".*", false)) { On 2012/06/27 20:28:00, Alexei Svitkine wrote: > Nit: 1 line if statements don't need braces. Done. http://codereview.chromium.org/10576003/diff/58001/chrome/browser/metrics/var... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10576003/diff/58001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:333: << study.filter().min_version(); On 2012/06/27 20:28:00, Alexei Svitkine wrote: > Nit: Align with << on the line above. Done. http://codereview.chromium.org/10576003/diff/58001/chrome/browser/metrics/var... chrome/browser/metrics/variations_service.cc:339: << study.filter().max_version(); On 2012/06/27 20:28:00, Alexei Svitkine wrote: > Nit: Align with << on the line above. Done.
(lgtm too)
owners LGTM, I didn't check the details. http://codereview.chromium.org/10576003/diff/61004/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/61004/base/version.cc#newcode63 base/version.cc:63: for (size_t i = count; i < components1.size(); ++i) Need {} for these for loops (since the content is > 1 line we require this). Same below.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@google.com/10576003/61004
http://codereview.chromium.org/10576003/diff/61004/base/version.cc File base/version.cc (right): http://codereview.chromium.org/10576003/diff/61004/base/version.cc#newcode63 base/version.cc:63: for (size_t i = count; i < components1.size(); ++i) On 2012/07/03 19:45:28, brettw wrote: > Need {} for these for loops (since the content is > 1 line we require this). > Same below. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@google.com/10576003/61007
Change committed as 145468 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||