Delete enumerations at Simple Cache backend deletion.

net/disk_cache/simple/simple_backend_impl.cc

Index: net/disk_cache/simple/simple_backend_impl.cc
diff --git a/net/disk_cache/simple/simple_backend_impl.cc b/net/disk_cache/simple/simple_backend_impl.cc
index c92a93a7114406d7003290d8b5ae55019d36502e..5b7f77961dc4e734c6d58840f91085cc32c7ed42 100644
--- a/net/disk_cache/simple/simple_backend_impl.cc
+++ b/net/disk_cache/simple/simple_backend_impl.cc
@@ -483,9 +483,10 @@ int SimpleBackendImpl::OpenNextEntry(void** iter,
 }
 
 void SimpleBackendImpl::EndEnumeration(void** iter) {
-  SimpleIndex::HashList* entry_list =
-      static_cast<SimpleIndex::HashList*>(*iter);
-  delete entry_list;
+  const ptrdiff_t ptrdiff_enumeration_id = reinterpret_cast<ptrdiff_t>(*iter);
+  const ActiveEnumerationMap::KeyType enumeration_id = ptrdiff_enumeration_id;

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

How about a helper to hide and self-document the reinterpret_cast ?

ActiveEnumerationMap::KeyType IteratorToActiveEnumerationKey(void*)

(Or any more readable name)

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

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.

+  DCHECK_EQ(enumeration_id, ptrdiff_enumeration_id);
+  active_enumerations_.Remove(enumeration_id);
   *iter = NULL;
 }
 
@@ -623,11 +624,21 @@ void SimpleBackendImpl::GetNextEntryInIterator(
     callback.Run(error_code);
     return;
   }
+  std::vector<uint64>* entry_list = NULL;

[clamy, 2014/09/03 19:47:48]

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?

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

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.

   if (*iter == NULL) {
-    *iter = index()->GetAllHashes().release();
+    const ActiveEnumerationMap::KeyType new_enumeration_id =
+        active_enumerations_.Add(
+            entry_list = index()->GetAllHashes().release());
+    const ptrdiff_t ptrdiff_new_enumeration_id = new_enumeration_id;
+    DCHECK_EQ(new_enumeration_id, ptrdiff_new_enumeration_id);
+    *iter = reinterpret_cast<void*>(ptrdiff_new_enumeration_id);
+  } else {
+    const ptrdiff_t ptrdiff_enumeration_id =
+        reinterpret_cast<ptrdiff_t>(*iter);
+    const ActiveEnumerationMap::KeyType enumeration_id = ptrdiff_enumeration_id;

[clamy, 2014/09/03 19:47:48]

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?

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

I will do this via a helper as jsbell suggested.

+    DCHECK_EQ(ptrdiff_enumeration_id, enumeration_id);
+    entry_list = active_enumerations_.Lookup(enumeration_id);
   }
-  SimpleIndex::HashList* entry_list =
-      static_cast<SimpleIndex::HashList*>(*iter);
   while (entry_list->size() > 0) {
     uint64 entry_hash = entry_list->back();
     entry_list->pop_back();