MD5Update function uses StringPiece instead of raw buffer.

MD5Update function uses StringPiece instead of raw buffer.

BUG=none
TEST=base_unittests --gtest_filter=MD5.ContextWithStringData*


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

[dominich, 2011-07-20 17:39:22]

This is a speculative CL. If you approve, I would extend it to cover more types
as necessary (string16, std::vector, etc) and replace the calling sites.

[brettw, 2011-07-20 17:46:23]

I'd rather not do the templates. If the current API is inconvenient, I'm happy
changing it to a StringPiece and updating the callers.

[dominich, 2011-07-20 17:54:36]

On 2011/07/20 17:46:23, brettw wrote:
> I'd rather not do the templates. If the current API is inconvenient, I'm happy
> changing it to a StringPiece and updating the callers.

Fair enough - How about just adding MD5UpdateString that takes a StringPiece
instead of changing the existing one?

[brettw, 2011-07-20 17:58:35]

We should be trying to make the code more simple and easy to understand. Adding
more versions of functions does the opposite, so except in particularly strong
cases (there are 100's of callers, which this certainly isn't one) I think we
should avoid being lazy and update all the callers.

[Evan Martin, 2011-07-20 18:03:35]

On 2011/07/20 17:58:35, brettw wrote:
> We should be trying to make the code more simple and easy to understand.
Adding
> more versions of functions does the opposite, so except in particularly strong
> cases (there are 100's of callers, which this certainly isn't one) I think we
> should avoid being lazy and update all the callers.

I agree with Brett.  It looks like this function only has ~10 callsites.

http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=MD5U...

[dominich, 2011-07-20 18:10:07]

On 2011/07/20 17:58:35, brettw wrote:
> We should be trying to make the code more simple and easy to understand.
Adding
> more versions of functions does the opposite, so except in particularly strong
> cases (there are 100's of callers, which this certainly isn't one) I think we
> should avoid being lazy and update all the callers.

I was still planning to update the callers that are using string data. There are
a couple of callers that use non-string data and converting that data to be a
StringPiece will complicate the code (eg safe_browsing/prefix_set.cc that calls
with a std::vector<std::pair<SBPrefix, int>>).

If moving all callers to StringPiece is not feasible or desirable, and I don't
think it is, then I think adding a StringPiece-specific method and updating all
callers that can to that is the right thing to do.

[Evan Martin, 2011-07-20 18:18:16]

On 2011/07/20 18:10:07, dominich wrote:
> On 2011/07/20 17:58:35, brettw wrote:
> > We should be trying to make the code more simple and easy to understand.
> Adding
> > more versions of functions does the opposite, so except in particularly
strong
> > cases (there are 100's of callers, which this certainly isn't one) I think
we
> > should avoid being lazy and update all the callers.
> 
> I was still planning to update the callers that are using string data. There
are
> a couple of callers that use non-string data and converting that data to be a
> StringPiece will complicate the code (eg safe_browsing/prefix_set.cc that
calls
> with a std::vector<std::pair<SBPrefix, int>>).

!!!

Looks like this is the code:

  std::vector<std::pair<SBPrefix,size_t> > index;
...

  // Read the index vector.  Herb Sutter indicates that vectors are
  // guaranteed to be contiuguous, so reading to where element 0 lives
  // is valid.
  index.resize(header.index_size);
  read = fread(&(index[0]), sizeof(index[0]), index.size(), file.get());
  if (read != index.size())
    return NULL;
  MD5Update(&context, &(index[0]), index_bytes);


That seems pretty scary to me, but if we really do want to do that,
  MD5Update(&context, StringPiece(reinterpret_cast<char*>(&index[0]),
index_bytes)
will do the same thing.  The reinterp_cast there actually helps document why
this code is suspicous (the old code does the same thing implicitly anyway).

[dominich, 2011-07-20 18:24:03]

On 2011/07/20 18:18:16, Evan Martin wrote:
> On 2011/07/20 18:10:07, dominich wrote:
> > On 2011/07/20 17:58:35, brettw wrote:
> > > We should be trying to make the code more simple and easy to understand.
> > Adding
> > > more versions of functions does the opposite, so except in particularly
> strong
> > > cases (there are 100's of callers, which this certainly isn't one) I think
> we
> > > should avoid being lazy and update all the callers.
> > 
> > I was still planning to update the callers that are using string data. There
> are
> > a couple of callers that use non-string data and converting that data to be
a
> > StringPiece will complicate the code (eg safe_browsing/prefix_set.cc that
> calls
> > with a std::vector<std::pair<SBPrefix, int>>).
> 
> !!!
> 
> Looks like this is the code:
> 
>   std::vector<std::pair<SBPrefix,size_t> > index;
> ...
> 
>   // Read the index vector.  Herb Sutter indicates that vectors are
>   // guaranteed to be contiuguous, so reading to where element 0 lives
>   // is valid.
>   index.resize(header.index_size);
>   read = fread(&(index[0]), sizeof(index[0]), index.size(), file.get());
>   if (read != index.size())
>     return NULL;
>   MD5Update(&context, &(index[0]), index_bytes);
> 
> 
> That seems pretty scary to me, but if we really do want to do that,
>   MD5Update(&context, StringPiece(reinterpret_cast<char*>(&index[0]),
> index_bytes)
> will do the same thing.  The reinterp_cast there actually helps document why
> this code is suspicous (the old code does the same thing implicitly anyway).

That's a good point. I'll just switch everything over then. Thanks.

[dominich, 2011-07-20 18:26:56]

On 2011/07/20 18:24:03, dominich wrote:
> On 2011/07/20 18:18:16, Evan Martin wrote:
> > On 2011/07/20 18:10:07, dominich wrote:
> > > On 2011/07/20 17:58:35, brettw wrote:
> > > > We should be trying to make the code more simple and easy to understand.
> > > Adding
> > > > more versions of functions does the opposite, so except in particularly
> > strong
> > > > cases (there are 100's of callers, which this certainly isn't one) I
think
> > we
> > > > should avoid being lazy and update all the callers.
> > > 
> > > I was still planning to update the callers that are using string data.
There
> > are
> > > a couple of callers that use non-string data and converting that data to
be
> a
> > > StringPiece will complicate the code (eg safe_browsing/prefix_set.cc that
> > calls
> > > with a std::vector<std::pair<SBPrefix, int>>).
> > 
> > !!!
> > 
> > Looks like this is the code:
> > 
> >   std::vector<std::pair<SBPrefix,size_t> > index;
> > ...
> > 
> >   // Read the index vector.  Herb Sutter indicates that vectors are
> >   // guaranteed to be contiuguous, so reading to where element 0 lives
> >   // is valid.
> >   index.resize(header.index_size);
> >   read = fread(&(index[0]), sizeof(index[0]), index.size(), file.get());
> >   if (read != index.size())
> >     return NULL;
> >   MD5Update(&context, &(index[0]), index_bytes);
> > 
> > 
> > That seems pretty scary to me, but if we really do want to do that,
> >   MD5Update(&context, StringPiece(reinterpret_cast<char*>(&index[0]),
> > index_bytes)
> > will do the same thing.  The reinterp_cast there actually helps document why
> > this code is suspicous (the old code does the same thing implicitly anyway).
> 
> That's a good point. I'll just switch everything over then. Thanks.

While I'm in there, any objections to me re-indenting and getting rid of things
like register variables, just updating the code in general?

[Evan Martin, 2011-07-20 18:28:16]

> While I'm in there, any objections to me re-indenting and getting rid of
things
> like register variables, just updating the code in general?

I wonder if this code came from google3, in which case there's maybe some
argument to keep it matching.

If not, I have no objection, as long as your cleanup change is a separate review
from any functional changes.  :)

[dominich, 2011-07-20 18:33:10]

On 2011/07/20 18:28:16, Evan Martin wrote:
> > While I'm in there, any objections to me re-indenting and getting rid of
> things
> > like register variables, just updating the code in general?
> 
> I wonder if this code came from google3, in which case there's maybe some
> argument to keep it matching.

It looks like this same code has spread around a bit in the last 18 years:
https://cs.corp.google.com/#search/&q=MD5Update%20lang:%5Ec%2B%2B$&sq=package...

So there could be an argument for not touching it, but equally it could be time
to update it to Chromium style.

> If not, I have no objection, as long as your cleanup change is a separate
review
> from any functional changes.  :)

Got it.

[brettw, 2011-07-20 20:26:28]

http://codereview.chromium.org/7466003/diff/3002/base/md5.cc
File base/md5.cc (right):

http://codereview.chromium.org/7466003/diff/3002/base/md5.cc#newcode214
base/md5.cc:214: void MD5Update(MD5Context* context, const StringPiece& data) {
I think you should change the original function rather than having this useless
pass-through here.

[dominich, 2011-07-20 22:14:20]

Removed the wrapper function and changed MD5String to use StringPiece for
consistency.

I would also change MD5Sum however it is used by third_party. I'm assuming that
these packages are occasionally updated and changing code in there to diverge
the packages from their source is bad idea. Please let me know if this is not
the case and I'll switch MD5Sum too.

[brettw, 2011-07-21 17:06:26]

LGTM

[commit-bot: I haz the power, 2011-07-21 19:29:10]

Presubmit check for 7466003-3018 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  base/md5.cc

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
net/http/http_vary_data.cc,chrome/browser/sync/util/user_settings.cc

Presubmit checks took 1.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[dominich, 2011-07-21 19:37:52]

Adding owners for net/ and chrome/browser/sync.

gavinp: please look at the net/ changes
akalin: please look at the chrome/browser/sync changes.

[akalin, 2011-07-21 19:49:57]

sync changes LGTM

[dominich, 2011-07-21 20:52:08]

cbentzel: As gavinp is out could you look at the net/ changes?

[cbentzel, 2011-07-21 20:57:46]

net LGTM

[gavinp, 2011-07-21 22:16:26]

net changes LGTM.  I'm not on vacation until tomorrow though...

[tfarina, 2011-07-21 22:18:54]

http://codereview.chromium.org/7466003/diff/10001/base/md5_unittest.cc
File base/md5_unittest.cc (right):

http://codereview.chromium.org/7466003/diff/10001/base/md5_unittest.cc#newcod...
base/md5_unittest.cc:197: std::string expected =
"900150983cd24fb0d6963f7d28e17f72";
could you move expected near of |actual| below?

http://codereview.chromium.org/7466003/diff/10001/base/md5_unittest.cc#newcod...
base/md5_unittest.cc:199: MD5Update(&ctx, data);
can you pass "abc" directly?

http://codereview.chromium.org/7466003/diff/10001/base/md5_unittest.cc#newcod...
base/md5_unittest.cc:209: 
remove this extra blank line.

[tfarina, 2011-07-21 22:21:24]

http://codereview.chromium.org/7466003/diff/10001/base/md5.cc
File base/md5.cc (right):

http://codereview.chromium.org/7466003/diff/10001/base/md5.cc#newcode4
base/md5.cc:4: // Copyright (c) 2011 The Chromium Authors. All rights reserved.
Could you put this as the first three lines, then the original copyright?

http://codereview.chromium.org/7466003/diff/10001/base/md5.cc#newcode172
base/md5.cc:172: const unsigned char* inbuf = (const unsigned char*)
data.data();
no space between (const unsigned char*) and data.data()?

[commit-bot: I haz the power, 2011-07-22 03:56:30]

Commit queue failed due to updated description.

[dominich, 2011-07-25 20:58:27]

http://codereview.chromium.org/7466003/diff/10001/base/md5.cc
File base/md5.cc (right):

http://codereview.chromium.org/7466003/diff/10001/base/md5.cc#newcode4
base/md5.cc:4: // Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2011/07/21 22:21:24, tfarina wrote:
> Could you put this as the first three lines, then the original copyright?

Done.

http://codereview.chromium.org/7466003/diff/10001/base/md5.cc#newcode172
base/md5.cc:172: const unsigned char* inbuf = (const unsigned char*)
data.data();
On 2011/07/21 22:21:24, tfarina wrote:
> no space between (const unsigned char*) and data.data()?

Done.

http://codereview.chromium.org/7466003/diff/10001/base/md5_unittest.cc
File base/md5_unittest.cc (right):

http://codereview.chromium.org/7466003/diff/10001/base/md5_unittest.cc#newcod...
base/md5_unittest.cc:197: std::string expected =
"900150983cd24fb0d6963f7d28e17f72";
On 2011/07/21 22:18:54, tfarina wrote:
> could you move expected near of |actual| below?

Done.

http://codereview.chromium.org/7466003/diff/10001/base/md5_unittest.cc#newcod...
base/md5_unittest.cc:199: MD5Update(&ctx, data);
On 2011/07/21 22:18:54, tfarina wrote:
> can you pass "abc" directly?

Done.

http://codereview.chromium.org/7466003/diff/10001/base/md5_unittest.cc#newcod...
base/md5_unittest.cc:209: 
On 2011/07/21 22:18:54, tfarina wrote:
> remove this extra blank line.

Done.

[brettw, 2011-07-26 04:35:58]

still LGTM

http://codereview.chromium.org/7466003/diff/16001/base/md5.cc
File base/md5.cc (right):

http://codereview.chromium.org/7466003/diff/16001/base/md5.cc#newcode6
base/md5.cc:6: // Modifications Copyright 2006 Google Inc. All Rights Reserved
Just remove the "Modifications Copyright 2006 Google Inc. All Rights Reserved",
our modifications should be licensed by the Chromium BSD license. Thanks for
cleaning this up.

[commit-bot: I haz the power, 2011-07-26 23:39:53]

Change committed as 94203