[BLE Refactor] Fixed BloomFilterBase base member _bits is not initialized when construct from a non-empty ByteArray.

PiperOrigin-RevId: 436229874

connections/implementation/mediums/ble_v2.cc

@@ -390,9 +390,9 @@ ByteArray BleV2::CreateAdvertisementHeader() {
       Utils::GenerateRandomBytes(kDummyServiceIdLength);
   std::string dummy_service_id{dummy_service_id_bytes};
 
-  mediums::BloomFilter<
-      mediums::BleAdvertisementHeader::kServiceIdBloomFilterLength>
-      bloom_filter;
+  mediums::BloomFilter bloom_filter(
+      std::make_unique<mediums::BitSetImpl<
+          mediums::BleAdvertisementHeader::kServiceIdBloomFilterLength>>());
   bloom_filter.Add(dummy_service_id);
 
   ByteArray advertisement_hash =

connections/implementation/mediums/ble_v2/bloom_filter.cc

@@ -16,6 +16,7 @@
 
 #include "absl/numeric/int128.h"
 #include "absl/strings/numbers.h"
+#include "internal/platform/logging.h"
 #include "src/MurmurHash3.h"
 
 namespace location {
@@ -23,33 +24,43 @@ namespace nearby {
 namespace connections {
 namespace mediums {
 
-BloomFilterBase::BloomFilterBase(const ByteArray& bytes, BitSet* bit_set)
-    : bits_(bit_set) {
+namespace {
+constexpr int kHasherNumberOfRepetitions = 5;
+}
+
+BloomFilter::BloomFilter(std::unique_ptr<BitSet> bit_set,
+                         const ByteArray& bytes)
+    : bit_set_(std::move(bit_set)) {
   const char* bytes_read_ptr = bytes.data();
+
+  if (bytes.size() == 0) {
+    // Ignore it; we don't need to copy the bit for the empty bytes.
+    return;
+  }
+  // If the size is not matched, fall out.
+  if (bytes.size() * 8 != bit_set_->Size()) {
+    NEARBY_LOGS(INFO) << "Cannot construct from bytes since the size is not "
+                         "matched. bytes.size(x8) = "
+                      << bytes.size() << ", bit_set.size=" << bit_set_->Size();
+    return;
+  }
   for (size_t byte_index = 0; byte_index < bytes.size(); byte_index++) {
     for (size_t bit_index = 0; bit_index < 8; bit_index++) {
-      bits_->Set((byte_index * 8) + bit_index,
-                 (*bytes_read_ptr >> bit_index) & 0x01);
+      bit_set_->Set((byte_index * 8) + bit_index,
+                    (*bytes_read_ptr >> bit_index) & 0x01);
     }
     bytes_read_ptr++;
   }
 }
 
-BloomFilterBase::operator ByteArray() const {
-  // Gets a binary string representation of the bitset where the leftmost
-  // character corresponds to bitset position (total size) - 1.
-  //
-  // If the bitset's internal representation is:
-  //     [position 0]     0 0 1 1 0 0 0 1 0 1 0 1     [position 11]
-  // The string representation will be outputted like this:
-  //                     "1 0 1 0 1 0 0 0 1 1 0 0"
-  std::string bitset_binary_string = bits_->ToString();
+BloomFilter::operator ByteArray() const {
+  std::string bitset_binary_string = bit_set_->ToString();
 
   ByteArray result_bytes(GetMinBytesForBits());
   char* result_bytes_write_ptr = result_bytes.data();
   // We go through the string backwards because the rightmost character
   // corresponds to position 0 in the bitset.
-  for (size_t i = bits_->Size(); i > 0; i -= 8) {
+  for (size_t i = bit_set_->Size(); i > 0; i -= 8) {
     std::string byte_binary_string = bitset_binary_string.substr(i - 8, 8);
     std::uint32_t byte_value;
     absl::numbers_internal::safe_strtou32_base(byte_binary_string, &byte_value,
@@ -60,26 +71,26 @@ BloomFilterBase::operator ByteArray() const {
   return result_bytes;
 }
 
-void BloomFilterBase::Add(const std::string& s) {
+void BloomFilter::Add(const std::string& s) {
   std::vector<std::int32_t> hashes = GetHashes(s);
   for (int32_t hash : hashes) {
-    size_t position = static_cast<size_t>(hash) % bits_->Size();
-    bits_->Set(position, true);
+    size_t position = static_cast<size_t>(hash) % bit_set_->Size();
+    bit_set_->Set(position, true);
   }
 }
 
-bool BloomFilterBase::PossiblyContains(const std::string& s) {
+bool BloomFilter::PossiblyContains(const std::string& s) {
   std::vector<std::int32_t> hashes = GetHashes(s);
   for (int32_t hash : hashes) {
-    size_t position = static_cast<size_t>(hash) % bits_->Size();
-    if (!bits_->Test(position)) {
+    size_t position = static_cast<size_t>(hash) % bit_set_->Size();
+    if (!bit_set_->Test(position)) {
       return false;
     }
   }
   return true;
 }
 
-std::vector<std::int32_t> BloomFilterBase::GetHashes(const std::string& s) {
+std::vector<std::int32_t> BloomFilter::GetHashes(const std::string& s) {
   std::vector<std::int32_t> hashes(kHasherNumberOfRepetitions, 0);
 
   absl::uint128 hash128;

connections/implementation/mediums/ble_v2/bloom_filter.h

@@ -25,72 +25,72 @@ namespace nearby {
 namespace connections {
 namespace mediums {
 
-/**
- * A bloom filter that gives access to the underlying BitSet. The implementation
- * is copied from our Java version of Bloom filter, which in turn copies from
- * Guava's BloomFilter.
- *
- * BloomFilter is templatized on the size of the byte array and not the size of
- * the bit set to ensure the bit set's length is a multiple of 8 (and can
- * neatly be returned as a ByteArray).
- */
-class BloomFilterBase {
+// Interface to set bits of the given bit array, by inserting a user element.
+class BitSet {
  public:
+  virtual ~BitSet() = default;
+
+  // Gets a binary string representation of the bitset where the leftmost
+  // character corresponds to bitset position (total size) - 1.
+  //
+  // If the bitset's internal representation is:
+  //     [position 0]     0 0 1 1 0 0 0 1 0 1 0 1     [position 11]
+  // The string representation will be outputted like this:
+  //                     "1 0 1 0 1 0 0 0 1 1 0 0"
+  virtual std::string ToString() const = 0;
+  virtual void Set(size_t pos, bool value) = 0;
+  virtual bool Test(size_t pos) const = 0;
+  virtual size_t Size() const = 0;
+};
+
+// A bloom filter that gives access to the underlying BitSet. The implementation
+// is copied from our Java version of Bloom filter, which in turn copies from
+// Guava's BloomFilter.
+class BloomFilter {
+ public:
+  // Constructs by injecting BitSet implementation. The bit_set will be default
+  // zero-out.
+  explicit BloomFilter(std::unique_ptr<BitSet> bit_set)
+      : BloomFilter(std::move(bit_set), {}) {}
+
+  // Constructs by injecting BitSet implementation with bytes of other
+  // BloomFilter. The `bit_set` will be filled with the bit_set of other
+  // BloomFilter.
+  //
+  // Note: The capacity size of current bit_set should be the same as the one
+  // of other BloomFilter, or there is no impact and fallback to the first
+  // constructor.
+  BloomFilter(std::unique_ptr<BitSet> bit_set, const ByteArray& bytes);
+  BloomFilter(BloomFilter&& other) = default;
+  BloomFilter& operator=(BloomFilter&& other) = default;
+
   explicit operator ByteArray() const;
 
   void Add(const std::string& s);
   bool PossiblyContains(const std::string& s);
 
- protected:
-  class BitSet {
-   public:
-    virtual ~BitSet() = default;
-    virtual std::string ToString() const = 0;
-    virtual void Set(size_t pos, bool value) = 0;
-    virtual bool Test(size_t pos) const = 0;
-    virtual size_t Size() const = 0;
-  };
-
-  BloomFilterBase(const ByteArray& bytes, BitSet* bit_set);
-  virtual ~BloomFilterBase() = default;
-
-  constexpr static int kHasherNumberOfRepetitions = 5;
-  std::vector<std::int32_t> GetHashes(const std::string& s);
-
  private:
-  int GetMinBytesForBits() const { return (bits_->Size() + 7) >> 3; }
+  std::vector<std::int32_t> GetHashes(const std::string& s);
+  int GetMinBytesForBits() const { return (bit_set_->Size() + 7) >> 3; }
 
-  BitSet* bits_;
+  std::unique_ptr<BitSet> bit_set_;
 };
 
+// A default bit set implementation.
+//
+// It is templatized on the size of the byte array and not the size of
+// the bit set to ensure the bit set's length is a multiple of 8 (and can
+// neatly be returned as a ByteArray).
 template <size_t CapacityInBytes>
-class BloomFilter final : public BloomFilterBase {
+class BitSetImpl final : public BitSet {
  public:
-  BloomFilter() : BloomFilterBase(ByteArray{}, &bits_) {}
-  explicit BloomFilter(const ByteArray& bytes)
-      : BloomFilterBase(bytes, &bits_) {}
-  BloomFilter(const BloomFilter&) = default;
-  BloomFilter& operator=(const BloomFilter&) = default;
-  BloomFilter(BloomFilter&& other) : BloomFilterBase(ByteArray{}, &bits_) {
-    *this = std::move(other);
-  }
-  BloomFilter& operator=(BloomFilter&& other) {
-    std::swap((*this).bits_, other.bits_);
-    return *this;
-  }
-  ~BloomFilter() override = default;
+  std::string ToString() const override { return bits_.to_string(); }
+  void Set(size_t pos, bool value) override { bits_.set(pos, value); }
+  bool Test(size_t pos) const override { return bits_.test(pos); }
+  size_t Size() const override { return bits_.size(); }
 
  private:
-  class BitSetImpl final : public BitSet {
-   public:
-    std::string ToString() const override { return bits_.to_string(); }
-    void Set(size_t pos, bool value) override { bits_.set(pos, value); }
-    bool Test(size_t pos) const override { return bits_.test(pos); }
-    size_t Size() const override { return bits_.size(); }
-
-   private:
-    std::bitset<CapacityInBytes * 8> bits_;
-  } bits_;
+  std::bitset<CapacityInBytes * 8> bits_;
 };
 
 }  // namespace mediums

connections/implementation/mediums/ble_v2/bloom_filter_test.cc

@@ -27,7 +27,7 @@ namespace {
 constexpr size_t kByteArrayLength = 100;
 
 TEST(BloomFilterTest, EmptyFilterReturnsEmptyArray) {
-  BloomFilter<kByteArrayLength> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   ByteArray bloom_filter_bytes(bloom_filter);
   std::string empty_string(kByteArrayLength, '\0');
@@ -36,7 +36,7 @@ TEST(BloomFilterTest, EmptyFilterReturnsEmptyArray) {
 }
 
 TEST(BloomFilterTest, EmptyFilterNeverContains) {
-  BloomFilter<kByteArrayLength> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   EXPECT_FALSE(bloom_filter.PossiblyContains("ELEMENT_1"));
   EXPECT_FALSE(bloom_filter.PossiblyContains("ELEMENT_2"));
@@ -44,7 +44,7 @@ TEST(BloomFilterTest, EmptyFilterNeverContains) {
 }
 
 TEST(BloomFilterTest, AddSuccess) {
-  BloomFilter<kByteArrayLength> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   EXPECT_FALSE(bloom_filter.PossiblyContains("ELEMENT_1"));
 
@@ -54,7 +54,7 @@ TEST(BloomFilterTest, AddSuccess) {
 }
 
 TEST(BloomFilterTest, AddOnlyGivenArg) {
-  BloomFilter<kByteArrayLength> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   bloom_filter.Add("ELEMENT_1");
 
@@ -64,7 +64,7 @@ TEST(BloomFilterTest, AddOnlyGivenArg) {
 }
 
 TEST(BloomFilterTest, AddMultipleArgs) {
-  BloomFilter<kByteArrayLength> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   bloom_filter.Add("ELEMENT_1");
   bloom_filter.Add("ELEMENT_2");
@@ -75,7 +75,7 @@ TEST(BloomFilterTest, AddMultipleArgs) {
 }
 
 TEST(BloomFilterTest, AddMultipleArgsReturnsNonemptyArray) {
-  BloomFilter<10> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<10>>());
 
   bloom_filter.Add("ELEMENT_1");
   bloom_filter.Add("ELEMENT_2");
@@ -87,37 +87,22 @@ TEST(BloomFilterTest, AddMultipleArgsReturnsNonemptyArray) {
   EXPECT_NE(std::string(bloom_filter_bytes), empty_string);
 }
 
-TEST(BloomFilterTest, CopyConstructorAndAssignmentSuccess) {
-  BloomFilter<kByteArrayLength> bloom_filter;
-
-  EXPECT_FALSE(bloom_filter.PossiblyContains("ELEMENT_1"));
-
-  bloom_filter.Add("ELEMENT_1");
-
-  BloomFilter<kByteArrayLength> bloom_filter_copy_1{bloom_filter};
-  BloomFilter<kByteArrayLength> bloom_filter_copy_2 = bloom_filter;
-
-  EXPECT_TRUE(bloom_filter.PossiblyContains("ELEMENT_1"));
-  EXPECT_TRUE(bloom_filter_copy_1.PossiblyContains("ELEMENT_1"));
-  EXPECT_TRUE(bloom_filter_copy_2.PossiblyContains("ELEMENT_1"));
-}
-
 TEST(BloomFilterTest, MoveConstructorSuccess) {
-  BloomFilter<kByteArrayLength> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   bloom_filter.Add("ELEMENT_1");
 
-  BloomFilter<kByteArrayLength> bloom_filter_move{std::move(bloom_filter)};
+  BloomFilter bloom_filter_move{std::move(bloom_filter)};
 
   EXPECT_TRUE(bloom_filter_move.PossiblyContains("ELEMENT_1"));
 }
 
 TEST(BloomFilterTest, MoveAssignmentSuccess) {
-  BloomFilter<kByteArrayLength> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   bloom_filter.Add("ELEMENT_1");
 
-  BloomFilter<kByteArrayLength> bloom_filter_move = std::move(bloom_filter);
+  BloomFilter bloom_filter_move = std::move(bloom_filter);
 
   EXPECT_TRUE(bloom_filter_move.PossiblyContains("ELEMENT_1"));
 }
@@ -134,7 +119,7 @@ TEST(BloomFilterTest, MoveAssignmentSuccess) {
  * something like [ 0, 1, 0, 0, 1, 1, 0, 0, 0, 1, ..., 1, 0].
  */
 TEST(BloomFilterTest, RandomnessNoEndBias) {
-  BloomFilter<kByteArrayLength> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   // Add one element to our BloomFilter.
   bloom_filter.Add("ELEMENT_1");
@@ -178,7 +163,7 @@ TEST(BloomFilterTest, RandomnessNoEndBias) {
 }
 
 TEST(BloomFilterTest, RandomnessFalsePositiveRate) {
-  BloomFilter<10> bloom_filter;
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
 
   // Add 5 distinct elements to the BloomFilter.
   bloom_filter.Add("ELEMENT_1");
@@ -200,6 +185,34 @@ TEST(BloomFilterTest, RandomnessFalsePositiveRate) {
   EXPECT_LE(false_positives, 5);
 }
 
+TEST(BloomFilterTest, ConstructWithNonEmptyByteArrayWorks) {
+  BloomFilter bloom_filter(std::make_unique<BitSetImpl<kByteArrayLength>>());
+
+  bloom_filter.Add("ELEMENT_1");
+  ByteArray original_bloom_filter_bytes(bloom_filter);
+
+  BloomFilter bloom_filter_inherited(
+      std::make_unique<BitSetImpl<kByteArrayLength>>(),
+      original_bloom_filter_bytes);
+
+  EXPECT_TRUE(bloom_filter_inherited.PossiblyContains("ELEMENT_1"));
+}
+
+TEST(BloomFilterTest, ConstructLongByteArrayFails) {
+  // Make 1 more byte in original BloomFilter.
+  BloomFilter bloom_filter(
+      std::make_unique<BitSetImpl<kByteArrayLength + 1>>());
+
+  bloom_filter.Add("ELEMENT_1");
+  ByteArray original_bloom_filter_bytes(bloom_filter);
+
+  BloomFilter bloom_filter_inherited(
+      std::make_unique<BitSetImpl<kByteArrayLength>>(),
+      original_bloom_filter_bytes);
+
+  EXPECT_FALSE(bloom_filter_inherited.PossiblyContains("ELEMENT_1"));
+}
+
 }  // namespace
 }  // namespace mediums
 }  // namespace connections