Fixed bug where an empty version string is considered valid.

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42681

[akalin, 2010-03-25 20:35:15]

+erikkay for review

[Erik does not do reviews, 2010-03-25 20:43:29]

LGTM

http://codereview.chromium.org/1364002/diff/2001/3001
File base/version.cc (right):

http://codereview.chromium.org/1364002/diff/2001/3001#newcode75
base/version.cc:75: if (numbers.empty()) {
nit: remove {}

[Scott Hess - ex-Googler, 2010-03-25 20:47:29]

lgtm, minor nits.

http://codereview.chromium.org/1364002/diff/2001/3001
File base/version.cc (right):

http://codereview.chromium.org/1364002/diff/2001/3001#newcode7
base/version.cc:7: #include <vector>
vector is already in version.h

http://codereview.chromium.org/1364002/diff/2001/3001#newcode38
base/version.cc:38: DCHECK(is_valid_);
perhaps DCHECK(other.is_valid_);

http://codereview.chromium.org/1364002/diff/2001/3001#newcode39
base/version.cc:39: std::vector<uint16> other_components = other.components();
Any complaints about const std::vector<uint16>& while you're in here?  I think
that will remove the need to copy the vector (not that it's a huge issue).

http://codereview.chromium.org/1364002/diff/2001/3001#newcode77
base/version.cc:77: }
Style of this file seems to be no braces for one-line if() clause.

http://codereview.chromium.org/1364002/diff/2001/3003
File base/version_unittest.cc (right):

http://codereview.chromium.org/1364002/diff/2001/3003#newcode23
base/version_unittest.cc:23: {"", 0, false},
Add some variants on empty, like a space.  Those will be handled by the existing
code, but while you're here...

[akalin, 2010-03-25 21:02:38]

All comments done, submitting when trybots go green

http://codereview.chromium.org/1364002/diff/2001/3001
File base/version.cc (right):

http://codereview.chromium.org/1364002/diff/2001/3001#newcode7
base/version.cc:7: #include <vector>
removed.

On 2010/03/25 20:47:29, shess wrote:
> vector is already in version.h

http://codereview.chromium.org/1364002/diff/2001/3001#newcode38
base/version.cc:38: DCHECK(is_valid_);
On 2010/03/25 20:47:29, shess wrote:
> perhaps DCHECK(other.is_valid_);

Done.

http://codereview.chromium.org/1364002/diff/2001/3001#newcode39
base/version.cc:39: std::vector<uint16> other_components = other.components();
Yeah, that copy has always been unnecessary.  Changed to use other.components_
directly.

On 2010/03/25 20:47:29, shess wrote:
> Any complaints about const std::vector<uint16>& while you're in here?  I think
> that will remove the need to copy the vector (not that it's a huge issue).

http://codereview.chromium.org/1364002/diff/2001/3001#newcode75
base/version.cc:75: if (numbers.empty()) {
On 2010/03/25 20:43:29, Erik Kay wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/1364002/diff/2001/3001#newcode77
base/version.cc:77: }
On 2010/03/25 20:47:29, shess wrote:
> Style of this file seems to be no braces for one-line if() clause.

Done.

http://codereview.chromium.org/1364002/diff/2001/3003
File base/version_unittest.cc (right):

http://codereview.chromium.org/1364002/diff/2001/3003#newcode23
base/version_unittest.cc:23: {"", 0, false},
On 2010/03/25 20:47:29, shess wrote:
> Add some variants on empty, like a space.  Those will be handled by the
existing
> code, but while you're here...

Done.

[akalin, 2010-03-25 21:41:20]

trybots are acting up, but it looks like base_unittests is compiling/passing and
the errors are unrelated.  Submitting...

On 2010/03/25 21:02:38, akalin wrote:
> All comments done, submitting when trybots go green
> 
> http://codereview.chromium.org/1364002/diff/2001/3001
> File base/version.cc (right):
> 
> http://codereview.chromium.org/1364002/diff/2001/3001#newcode7
> base/version.cc:7: #include <vector>
> removed.
> 
> On 2010/03/25 20:47:29, shess wrote:
> > vector is already in version.h
> 
> http://codereview.chromium.org/1364002/diff/2001/3001#newcode38
> base/version.cc:38: DCHECK(is_valid_);
> On 2010/03/25 20:47:29, shess wrote:
> > perhaps DCHECK(other.is_valid_);
> 
> Done.
> 
> http://codereview.chromium.org/1364002/diff/2001/3001#newcode39
> base/version.cc:39: std::vector<uint16> other_components = other.components();
> Yeah, that copy has always been unnecessary.  Changed to use other.components_
> directly.
> 
> On 2010/03/25 20:47:29, shess wrote:
> > Any complaints about const std::vector<uint16>& while you're in here?  I
think
> > that will remove the need to copy the vector (not that it's a huge issue).
> 
> http://codereview.chromium.org/1364002/diff/2001/3001#newcode75
> base/version.cc:75: if (numbers.empty()) {
> On 2010/03/25 20:43:29, Erik Kay wrote:
> > nit: remove {}
> 
> Done.
> 
> http://codereview.chromium.org/1364002/diff/2001/3001#newcode77
> base/version.cc:77: }
> On 2010/03/25 20:47:29, shess wrote:
> > Style of this file seems to be no braces for one-line if() clause.
> 
> Done.
> 
> http://codereview.chromium.org/1364002/diff/2001/3003
> File base/version_unittest.cc (right):
> 
> http://codereview.chromium.org/1364002/diff/2001/3003#newcode23
> base/version_unittest.cc:23: {"", 0, false},
> On 2010/03/25 20:47:29, shess wrote:
> > Add some variants on empty, like a space.  Those will be handled by the
> existing
> > code, but while you're here...
> 
> Done.