Delete enumerations at Simple Cache backend deletion.

Delete enumerations at Simple Cache backend deletion.

The memory allocated for an enumeration should be freed when a backend
is deleted.

R=clamy@chromium.org,jkarlin@chromium.org
BUG=410276
Committed: https://crrev.com/48f632fe338f4c0a5b7d48fb865cf0c6af52ab47
Cr-Commit-Position: refs/heads/master@{#294705}

[gavinp, 2014-09-03 19:11:56]

clamy: PTAL.

jkarlin: FYI. Good find.

[clamy, 2014-09-03 19:47:49]

Thanks! Lgtm, with two nits.

https://chromiumcodereview.appspot.com/533293002/diff/20001/net/disk_cache/si...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://chromiumcodereview.appspot.com/533293002/diff/20001/net/disk_cache/si...
net/disk_cache/simple/simple_backend_impl.cc:627: std::vector<uint64>*
entry_list = NULL;
nit: The previous version used the type SimpleIndex::HashList. Do you think a
comment should be added to make it clear that we are operating on a list of
hashes?

https://chromiumcodereview.appspot.com/533293002/diff/20001/net/disk_cache/si...
net/disk_cache/simple/simple_backend_impl.cc:638: const
ActiveEnumerationMap::KeyType enumeration_id = ptrdiff_enumeration_id;
This is really ugly, but I guess we don't really have a choice given the API.
AFAIU, the ptrdiff_t is here because we could be trying to cast a 64-bit pointer
to ActiveEnumerationMap::KeyType which is an int32, right? Do you think this
should be underlined in a comment?

[jsbell, 2014-09-03 22:30:41]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2014-09-03 22:30:42]

drive-by...

https://codereview.chromium.org/533293002/diff/20001/net/disk_cache/simple/si...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/533293002/diff/20001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_backend_impl.cc:487: const
ActiveEnumerationMap::KeyType enumeration_id = ptrdiff_enumeration_id;
How about a helper to hide and self-document the reinterpret_cast ?

ActiveEnumerationMap::KeyType IteratorToActiveEnumerationKey(void*)

(Or any more readable name)

[gavinp, 2014-09-04 17:48:42]

I've decided that pointer casting is really ugly, and to write C++ instead. See
https://codereview.chromium.org/542733002/ , which obsoletes this CL.

[gavinp, 2014-09-12 20:22:20]

On 2014/09/04 17:48:42, gavinp wrote:
> I've decided that pointer casting is really ugly, and to write C++ instead.
See
> https://codereview.chromium.org/542733002/ , which obsoletes this CL.

This CL is now alive, because of surprising controversy in that CL.

[gavinp, 2014-09-12 21:01:04]

ty jsbell for your review, and for your contributions to moving forward our
discussion on https://codereview.chromium.org/542733002/

https://codereview.chromium.org/533293002/diff/20001/net/disk_cache/simple/si...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/533293002/diff/20001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_backend_impl.cc:487: const
ActiveEnumerationMap::KeyType enumeration_id = ptrdiff_enumeration_id;
On 2014/09/03 22:30:42, jsbell wrote:
> How about a helper to hide and self-document the reinterpret_cast ?
> 
> ActiveEnumerationMap::KeyType IteratorToActiveEnumerationKey(void*)
> 
> (Or any more readable name)

Good idea. I threw in some COMPILE_ASSERTS around the sizes of ptrdiff_t that
connect the behaviour to the guarantees made in the spec about casting void* to
and from integer types.

https://codereview.chromium.org/533293002/diff/20001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_backend_impl.cc:627: std::vector<uint64>*
entry_list = NULL;
On 2014/09/03 19:47:48, clamy wrote:
> nit: The previous version used the type SimpleIndex::HashList. Do you think a
> comment should be added to make it clear that we are operating on a list of
> hashes?

No. Using SimpleIndex::HashList was actually the odd thing; lots of places use
vector<uint64> to mean a bunch of hashes, so I think the comment is overkill. I
think the HashList typedef is actually the bad thing here; so being free of it
is what's good.

https://codereview.chromium.org/533293002/diff/20001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_backend_impl.cc:638: const
ActiveEnumerationMap::KeyType enumeration_id = ptrdiff_enumeration_id;
On 2014/09/03 19:47:48, clamy wrote:
> This is really ugly, but I guess we don't really have a choice given the API.
> AFAIU, the ptrdiff_t is here because we could be trying to cast a 64-bit
pointer
> to ActiveEnumerationMap::KeyType which is an int32, right? Do you think this
> should be underlined in a comment?

I will do this via a helper as jsbell suggested.

[jsbell, 2014-09-12 21:16:49]

lgtm

[gavinp, 2014-09-12 21:49:12]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-09-12 21:53:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/533293002/60001

[commit-bot: I haz the power, 2014-09-13 00:46:43]

Committed patchset #4 (id:60001) as 6f69110b2167c4ec4d034890e1f358c9c1247cf4

[commit-bot: I haz the power, 2014-09-13 00:52:38]

Patchset 4 (id:??) landed as
https://crrev.com/48f632fe338f4c0a5b7d48fb865cf0c6af52ab47
Cr-Commit-Position: refs/heads/master@{#294705}