base::MD5IntermediateFinal() generates digest non-destructively.

base::MD5IntermediateFinal() generates digest non-destructively.

MD5Final() modifies the context, which prevents calculating an intermediate
digest while also continuing to build an overall context.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260731

[Scott Hess - ex-Googler, 2014-03-25 20:20:12]

Alternative to https://codereview.chromium.org/211503004/ which just cuts to
generating an intermediate digest.

[Ryan Sleevi, 2014-03-26 17:49:45]

https://codereview.chromium.org/211273009/diff/1/base/md5.h
File base/md5.h (right):

https://codereview.chromium.org/211273009/diff/1/base/md5.h#newcode62
base/md5.h:62: // an intermediate digest without affecting the original context.
Comment nit: This doesn't really clone (at least, not from the point of view of
the caller)

// Because MD5Final() destructively finalizes the context, preventing further
data from being hashed,
// MD5IntermediateFinal() is provided to allow callers to obtain the hash of all
the data so far, while
// permitting the further hashing of additional data with the original context.

[Scott Hess - ex-Googler, 2014-03-27 19:22:34]

I get the impression that the judges are leaning 57% towards this CL, now?  I
could merge them both to save someone time when they need MD5Clone() in 2018 or
so.

https://codereview.chromium.org/211273009/diff/1/base/md5.h
File base/md5.h (right):

https://codereview.chromium.org/211273009/diff/1/base/md5.h#newcode62
base/md5.h:62: // an intermediate digest without affecting the original context.
On 2014/03/26 17:49:46, Ryan Sleevi wrote:
> Comment nit: This doesn't really clone (at least, not from the point of view
of
> the caller)
> 
> // Because MD5Final() destructively finalizes the context, preventing further
> data from being hashed,
> // MD5IntermediateFinal() is provided to allow callers to obtain the hash of
all
> the data so far, while
> // permitting the further hashing of additional data with the original
context.

Yeah, I first had both in a single CL and let the context influence me.

I didn't like your comment, so I randomly rewrote it, and threw in a const to
make it clear.  Feel free to tell me "No, my version", because I'm pretty sure
that anyone who's reading the comment is already lost :-).

[awong, 2014-03-31 20:22:19]

I think we should go with this one. @sleevi: scream if you disagree.

@shess: sent you a couple small nits.

https://codereview.chromium.org/211273009/diff/50001/base/md5.cc
File base/md5.cc (right):

https://codereview.chromium.org/211273009/diff/50001/base/md5.cc#newcode264
base/md5.cc:264: * Clone the context and use the clone to generate the digest.
This should be an impl comment, not a function comment. Also explain why cloning
is necessary.

https://codereview.chromium.org/211273009/diff/50001/base/md5_unittest.cc
File base/md5_unittest.cc (right):

https://codereview.chromium.org/211273009/diff/50001/base/md5_unittest.cc#new...
base/md5_unittest.cc:222: const char kHeader[] = "header data";
static const char blah[]?

[Scott Hess - ex-Googler, 2014-03-31 20:48:26]

https://codereview.chromium.org/211273009/diff/50001/base/md5.cc
File base/md5.cc (right):

https://codereview.chromium.org/211273009/diff/50001/base/md5.cc#newcode264
base/md5.cc:264: * Clone the context and use the clone to generate the digest.
On 2014/03/31 20:22:19, awong wrote:
> This should be an impl comment, not a function comment. Also explain why
cloning
> is necessary.

I'll try, but the commenting style is breaking my brain.  [Dear lord!  It was
copied from sqlite?]  WDYT about simply having no comment at all?

https://codereview.chromium.org/211273009/diff/50001/base/md5_unittest.cc
File base/md5_unittest.cc (right):

https://codereview.chromium.org/211273009/diff/50001/base/md5_unittest.cc#new...
base/md5_unittest.cc:222: const char kHeader[] = "header data";
On 2014/03/31 20:22:19, awong wrote:
> static const char blah[]?

Done.

[awong, 2014-03-31 21:04:32]

https://codereview.chromium.org/211273009/diff/50001/base/md5.cc
File base/md5.cc (right):

https://codereview.chromium.org/211273009/diff/50001/base/md5.cc#newcode264
base/md5.cc:264: * Clone the context and use the clone to generate the digest.
On 2014/03/31 20:48:26, shess wrote:
> On 2014/03/31 20:22:19, awong wrote:
> > This should be an impl comment, not a function comment. Also explain why
> cloning
> > is necessary.
> 
> I'll try, but the commenting style is breaking my brain.  [Dear lord!  It was
> copied from sqlite?]  WDYT about simply having no comment at all?
I think it's critical to understand the MD5Final mutates the MD5Context* and
that this memcpy is "good enough" to fix things.

What about
/* MD5Final mutates the MD5Context*. Make a copy for generating the intermediate
value. */

[Scott Hess - ex-Googler, 2014-03-31 21:29:59]

https://codereview.chromium.org/211273009/diff/50001/base/md5.cc
File base/md5.cc (right):

https://codereview.chromium.org/211273009/diff/50001/base/md5.cc#newcode264
base/md5.cc:264: * Clone the context and use the clone to generate the digest.
On 2014/03/31 21:04:32, awong wrote:
> On 2014/03/31 20:48:26, shess wrote:
> > On 2014/03/31 20:22:19, awong wrote:
> > > This should be an impl comment, not a function comment. Also explain why
> > cloning
> > > is necessary.
> > 
> > I'll try, but the commenting style is breaking my brain.  [Dear lord!  It
was
> > copied from sqlite?]  WDYT about simply having no comment at all?
> I think it's critical to understand the MD5Final mutates the MD5Context* and
> that this memcpy is "good enough" to fix things.
> 
> What about
> /* MD5Final mutates the MD5Context*. Make a copy for generating the
intermediate
> value. */

Done.

[awong, 2014-03-31 21:34:41]

LGTM

ship it.

[Scott Hess - ex-Googler, 2014-03-31 21:36:53]

On 2014/03/31 21:34:41, awong wrote:
> LGTM
> 
> ship it.

Shipping.  I won't write too much code based on it, yet, just in case Ryan says
"NFW, revert."

[Scott Hess - ex-Googler, 2014-03-31 21:36:57]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2014-03-31 21:39:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/211273009/90001

[awong, 2014-03-31 21:40:17]

On Mon, Mar 31, 2014 at 2:36 PM, <shess@chromium.org> wrote:

> On 2014/03/31 21:34:41, awong wrote:
>
>> LGTM
>>
>
>  ship it.
>>
>
> Shipping.  I won't write too much code based on it, yet, just in case Ryan
> says
> "NFW, revert."
>

My response to that: poo.

Yes! I'm a mature grown-up!


https://codereview.chromium.org/211273009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2014-03-31 21:41:25]

lgtm

[commit-bot: I haz the power, 2014-04-01 01:31:41]

Change committed as 260731