Misc. minor cleanups in bloom filter code:

* Use assignment/casting in place of memcpy() where possible
* Avoid some unnecessary size_t->int casts by just using size_t
* Be more consistent about usage of SBPrefix type
* Write shorter statements/blocks if possible, e.g. "a -= (b + c);" instead of "a = a - b; a = a - c;"
* Update copyrights

BUG=none
TEST=none
Review URL: http://codereview.chromium.org/386009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@31773 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 58 insertions, 74 deletions

diff --git a/chrome/browser/safe_browsing/bloom_filter.cc b/chrome/browser/safe_browsing/bloom_filter.cc
index 45e1c572..55cb4da 100644
--- a/chrome/browser/safe_browsing/bloom_filter.cc
+++ b/chrome/browser/safe_browsing/bloom_filter.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -15,19 +15,19 @@ namespace {
 
 // The Jenkins 96 bit mix function:
 // http://www.concentric.net/~Ttwang/tech/inthash.htm
-uint32 HashMix(uint64 hash_key, uint32 c) {
+uint32 HashMix(BloomFilter::HashKey hash_key, uint32 c) {
   uint32 a = static_cast<uint32>(hash_key)       & 0xFFFFFFFF;
   uint32 b = static_cast<uint32>(hash_key >> 32) & 0xFFFFFFFF;
 
-  a = a - b;  a = a - c;  a = a ^ (c >> 13);
-  b = b - c;  b = b - a;  b = b ^ (a << 8);
-  c = c - a;  c = c - b;  c = c ^ (b >> 13);
-  a = a - b;  a = a - c;  a = a ^ (c >> 12);
-  b = b - c;  b = b - a;  b = b ^ (a << 16);
-  c = c - a;  c = c - b;  c = c ^ (b >> 5);
-  a = a - b;  a = a - c;  a = a ^ (c >> 3);
-  b = b - c;  b = b - a;  b = b ^ (a << 10);
-  c = c - a;  c = c - b;  c = c ^ (b >> 15);
+  a -= (b + c);  a ^= (c >> 13);
+  b -= (c + a);  b ^= (a << 8);
+  c -= (a + b);  c ^= (b >> 13);
+  a -= (b + c);  a ^= (c >> 12);
+  b -= (c + a);  b ^= (a << 16);
+  c -= (a + b);  c ^= (b >> 5);
+  a -= (b + c);  a ^= (c >> 3);
+  b -= (c + a);  b ^= (a << 10);
+  c -= (a + b);  c ^= (b >> 15);
 
   return c;
 }
@@ -43,7 +43,7 @@ BloomFilter::BloomFilter(int bit_size) {
   memset(data_.get(), 0, byte_size_);
 }
 
-BloomFilter::BloomFilter(char* data, int size, const std::vector<uint64>& keys)
+BloomFilter::BloomFilter(char* data, int size, const HashKeys& keys)
     : hash_keys_(keys) {
   byte_size_ = size;
   bit_size_ = byte_size_ * 8;
@@ -53,31 +53,21 @@ BloomFilter::BloomFilter(char* data, int size, const std::vector<uint64>& keys)
 BloomFilter::~BloomFilter() {
 }
 
-void BloomFilter::Insert(int hash_int) {
-  uint32 hash;
-  memcpy(&hash, &hash_int, sizeof(hash));
-  for (int i = 0; i < static_cast<int>(hash_keys_.size()); ++i) {
-    uint32 mix = HashMix(hash_keys_[i], hash);
-    uint32 index = mix % bit_size_;
-    int byte = index / 8;
-    int bit = index % 8;
-    data_.get()[byte] |= 1 << bit;
+void BloomFilter::Insert(SBPrefix hash) {
+  uint32 hash_uint32 = static_cast<uint32>(hash);
+  for (size_t i = 0; i < hash_keys_.size(); ++i) {
+    uint32 index = HashMix(hash_keys_[i], hash_uint32) % bit_size_;
+    data_[index / 8] |= 1 << (index % 8);
   }
 }
 
-bool BloomFilter::Exists(int hash_int) const {
-  uint32 hash;
-  memcpy(&hash, &hash_int, sizeof(hash));
-  for (int i = 0; i < static_cast<int>(hash_keys_.size()); ++i) {
-    uint32 mix = HashMix(hash_keys_[i], hash);
-    uint32 index = mix % bit_size_;
-    int byte = index / 8;
-    int bit = index % 8;
-    char data = data_.get()[byte];
-    if (!(data & (1 << bit)))
+bool BloomFilter::Exists(SBPrefix hash) const {
+  uint32 hash_uint32 = static_cast<uint32>(hash);
+  for (size_t i = 0; i < hash_keys_.size(); ++i) {
+    uint32 index = HashMix(hash_keys_[i], hash_uint32) % bit_size_;
+    if (!(data_[index / 8] & (1 << (index % 8))))
       return false;
   }
-
   return true;
 }
 
@@ -104,9 +94,9 @@ BloomFilter* BloomFilter::LoadFile(const FilePath& filter_name) {
   if (bytes_read != sizeof(num_keys) || num_keys < 1 || num_keys > kNumHashKeys)
     return NULL;
 
-  std::vector<uint64> hash_keys;
+  HashKeys hash_keys;
   for (int i = 0; i < num_keys; ++i) {
-    uint64 key;
+    HashKey key;
     bytes_read = filter.Read(reinterpret_cast<char*>(&key), sizeof(key), NULL);
     if (bytes_read != sizeof(key))
       return NULL;
diff --git a/chrome/browser/safe_browsing/bloom_filter.h b/chrome/browser/safe_browsing/bloom_filter.h
index 061e933..5d9bbf6 100644
--- a/chrome/browser/safe_browsing/bloom_filter.h
+++ b/chrome/browser/safe_browsing/bloom_filter.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
@@ -23,19 +23,23 @@
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "base/basictypes.h"
+#include "chrome/browser/safe_browsing/safe_browsing_util.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
 class BloomFilter : public base::RefCountedThreadSafe<BloomFilter> {
  public:
+  typedef uint64 HashKey;
+  typedef std::vector<HashKey> HashKeys;
+
   // Constructs an empty filter with the given size.
   explicit BloomFilter(int bit_size);
 
   // Constructs a filter from serialized data. This object owns the memory and
   // will delete it on destruction.
-  BloomFilter(char* data, int size, const std::vector<uint64>& keys);
+  BloomFilter(char* data, int size, const HashKeys& keys);
 
-  void Insert(int hash);
-  bool Exists(int hash) const;
+  void Insert(SBPrefix hash);
+  bool Exists(SBPrefix hash) const;
 
   const char* data() const { return data_.get(); }
   int size() const { return byte_size_; }
@@ -70,7 +74,7 @@ class BloomFilter : public base::RefCountedThreadSafe<BloomFilter> {
   scoped_array<char> data_;
 
   // Random keys used for hashing.
-  std::vector<uint64> hash_keys_;
+  HashKeys hash_keys_;
 
   DISALLOW_COPY_AND_ASSIGN(BloomFilter);
 };
diff --git a/chrome/browser/safe_browsing/bloom_filter_unittest.cc b/chrome/browser/safe_browsing/bloom_filter_unittest.cc
index f8dde07..bf3a123 100644
--- a/chrome/browser/safe_browsing/bloom_filter_unittest.cc
+++ b/chrome/browser/safe_browsing/bloom_filter_unittest.cc
@@ -1,7 +1,6 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
-//
 
 #include "chrome/browser/safe_browsing/bloom_filter.h"
 
@@ -20,24 +19,24 @@
 
 namespace {
 
-uint32 GenHash() {
-  return static_cast<uint32>(base::RandUint64());
+SBPrefix GenHash() {
+  return static_cast<SBPrefix>(base::RandUint64());
 }
 
 }
 
 TEST(SafeBrowsingBloomFilter, BloomFilterUse) {
   // Use a small number for unit test so it's not slow.
-  uint32 count = 1000;
+  int count = 1000;
 
   // Build up the bloom filter.
   scoped_refptr<BloomFilter> filter =
       new BloomFilter(count * BloomFilter::kBloomFilterSizeRatio);
 
-  typedef std::set<int> Values;
+  typedef std::set<SBPrefix> Values;
   Values values;
-  for (uint32 i = 0; i < count; ++i) {
-    uint32 value = GenHash();
+  for (int i = 0; i < count; ++i) {
+    SBPrefix value = GenHash();
     values.insert(value);
     filter->Insert(value);
   }
@@ -49,24 +48,23 @@ TEST(SafeBrowsingBloomFilter, BloomFilterUse) {
       new BloomFilter(data_copy, filter->size(), filter->hash_keys_);
 
   // Check no false negatives by ensuring that every time we inserted exists.
-  for (Values::iterator i = values.begin(); i != values.end(); ++i) {
+  for (Values::const_iterator i = values.begin(); i != values.end(); ++i)
     EXPECT_TRUE(filter_copy->Exists(*i));
-  }
 
   // Check false positive error rate by checking the same number of items that
   // we inserted, but of different values, and calculating what percentage are
   // "found".
-  uint32 found_count = 0;
-  uint32 checked = 0;
+  int found_count = 0;
+  int checked = 0;
   while (true) {
-    uint32 value = GenHash();
-    if (values.find(value) != values.end())
+    SBPrefix value = GenHash();
+    if (values.count(value))
       continue;
 
     if (filter_copy->Exists(value))
       found_count++;
 
-    checked ++;
+    checked++;
     if (checked == count)
       break;
   }
@@ -106,14 +104,13 @@ TEST(SafeBrowsingBloomFilter, BloomFilterFile) {
   // Check data consistency.
   EXPECT_EQ(filter_write->hash_keys_.size(), filter_read->hash_keys_.size());
 
-  for (int i = 0; i < static_cast<int>(filter_write->hash_keys_.size()); ++i)
+  for (size_t i = 0; i < filter_write->hash_keys_.size(); ++i)
     EXPECT_EQ(filter_write->hash_keys_[i], filter_read->hash_keys_[i]);
 
   EXPECT_EQ(filter_write->size(), filter_read->size());
 
-  EXPECT_TRUE(memcmp(filter_write->data(),
-                     filter_read->data(),
-                     filter_read->size()) == 0);
+  EXPECT_EQ(0,
+      memcmp(filter_write->data(), filter_read->data(), filter_read->size()));
 
   file_util::Delete(filter_path, false);
 }
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
index 98bc709..46fa2b7 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -201,9 +201,9 @@ bool SafeBrowsingDatabaseBloom::ResetDatabase() {
     return false;
   }
 
+  DeleteBloomFilter();
   bloom_filter_ = new BloomFilter(BloomFilter::kBloomFilterMinSize *
                                   BloomFilter::kBloomFilterSizeRatio);
-  file_util::Delete(bloom_filter_filename_, false);
 
   // TODO(paulg): Fix potential infinite recursion between Open and Reset.
   return Open();
@@ -271,8 +271,7 @@ bool SafeBrowsingDatabaseBloom::ContainsUrl(
       // and then fall back to the full hash if there's a hit.
       base::SHA256HashString(hosts[i] + paths[j], &full_hash,
                              sizeof(SBFullHash));
-      SBPrefix prefix;
-      memcpy(&prefix, &full_hash, sizeof(SBPrefix));
+      SBPrefix prefix = *reinterpret_cast<SBPrefix*>(&full_hash);
       if (bloom_filter_->Exists(prefix))
         prefix_hits->push_back(prefix);
     }
@@ -399,9 +398,8 @@ void SafeBrowsingDatabaseBloom::InsertAdd(SBPrefix host, SBEntry* entry) {
   if (entry->type() == SBEntry::ADD_FULL_HASH) {
     base::Time receive_time = base::Time::Now();
     for (int i = 0; i < entry->prefix_count(); ++i) {
-      SBPrefix prefix;
       SBFullHash full_hash = entry->FullHashAt(i);
-      memcpy(&prefix, full_hash.full_hash, sizeof(SBPrefix));
+      SBPrefix prefix = *reinterpret_cast<SBPrefix*>(&full_hash);
       InsertAddPrefix(prefix, encoded);
       InsertAddFullHash(prefix, encoded, receive_time, full_hash);
     }
@@ -476,9 +474,8 @@ void SafeBrowsingDatabaseBloom::InsertSub(
 
   if (entry->type() == SBEntry::SUB_FULL_HASH) {
     for (int i = 0; i < entry->prefix_count(); ++i) {
-      SBPrefix prefix;
       SBFullHash full_hash = entry->FullHashAt(i);
-      memcpy(&prefix, full_hash.full_hash, sizeof(SBPrefix));
+      SBPrefix prefix = *reinterpret_cast<SBPrefix*>(&full_hash);
       encoded_add = EncodeChunkId(entry->ChunkIdAtPrefix(i), entry->list_id());
       InsertSubPrefix(prefix, encoded, encoded_add);
       InsertSubFullHash(prefix, encoded, encoded_add, full_hash, false);
@@ -1072,8 +1069,7 @@ void SafeBrowsingDatabaseBloom::WriteFullHashList(const HashList& hash_list,
   HashList::const_iterator lit = hash_list.begin();
   for (; lit != hash_list.end(); ++lit) {
     const HashCacheEntry& entry = *lit;
-    SBPrefix prefix;
-    memcpy(&prefix, entry.full_hash.full_hash, sizeof(SBPrefix));
+    SBPrefix prefix = *reinterpret_cast<const SBPrefix*>(&entry.full_hash);
     if (is_add) {
       if (add_del_cache_.find(entry.add_chunk_id) == add_del_cache_.end()) {
         InsertAddFullHash(prefix, entry.add_chunk_id,
@@ -1340,9 +1336,7 @@ void SafeBrowsingDatabaseBloom::GetCachedFullHashes(
         // receive the next update (that doesn't sub it).
         if (max_age < last_update || eit->received > max_age) {
           SBFullHashResult full_hash;
-          memcpy(&full_hash.hash.full_hash,
-                 &eit->full_hash.full_hash,
-                 sizeof(SBFullHash));
+          full_hash.hash = eit->full_hash;
           full_hash.list_name = safe_browsing_util::GetListName(eit->list_id);
           full_hash.add_chunk_id = eit->add_chunk_id;
           full_hits->push_back(full_hash);
@@ -1374,14 +1368,13 @@ void SafeBrowsingDatabaseBloom::CacheHashResults(
   const Time now = Time::Now();
   for (std::vector<SBFullHashResult>::const_iterator it = full_hits.begin();
        it != full_hits.end(); ++it) {
-    SBPrefix prefix;
-    memcpy(&prefix, &it->hash.full_hash, sizeof(SBPrefix));
+    SBPrefix prefix = *reinterpret_cast<const SBPrefix*>(&it->hash);
     HashList& entries = (*hash_cache_)[prefix];
     HashCacheEntry entry;
     entry.received = now;
     entry.list_id = safe_browsing_util::GetListId(it->list_name);
     entry.add_chunk_id = EncodeChunkId(it->add_chunk_id, entry.list_id);
-    memcpy(&entry.full_hash, &it->hash.full_hash, sizeof(SBFullHash));
+    entry.full_hash = it->hash;
     entries.push_back(entry);
 
     // Also push a copy to the pending write queue.