From 004d20ca90f22789b190feb1a4ad3e8da0a3807a Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Tue, 9 Feb 2016 12:47:11 +0300 Subject: [PATCH] Review fixes. --- .../compressed_bit_vector_test.cpp | 22 +++++++++---------- coding/compressed_bit_vector.hpp | 2 ++ search/v2/features_filter.cpp | 10 +-------- search/v2/features_filter.hpp | 14 ++++++------ search/v2/geocoder.cpp | 2 +- search/v2/geocoder.hpp | 2 +- 6 files changed, 23 insertions(+), 29 deletions(-) diff --git a/coding/coding_tests/compressed_bit_vector_test.cpp b/coding/coding_tests/compressed_bit_vector_test.cpp index 3507ec0903..812a108a47 100644 --- a/coding/coding_tests/compressed_bit_vector_test.cpp +++ b/coding/coding_tests/compressed_bit_vector_test.cpp @@ -415,7 +415,7 @@ UNIT_TEST(CompressedBitVector_DenseLeaveFirstNBits) { vector setBits; - for (int i = 0; i < 100; ++i) + for (uint64_t i = 0; i < 100; ++i) setBits.push_back(2 * i); auto cbv = coding::CompressedBitVectorBuilder::FromBitPositions(setBits); TEST_EQUAL(cbv->PopCount(), 100, ()); @@ -425,7 +425,7 @@ UNIT_TEST(CompressedBitVector_DenseLeaveFirstNBits) TEST_EQUAL(cbv->PopCount(), 50, ()); TEST_EQUAL(cbv->GetStorageStrategy(), coding::CompressedBitVector::StorageStrategy::Dense, ()); - for (int i = 0; i < 50; ++i) + for (uint64_t i = 0; i < 50; ++i) { TEST(cbv->GetBit(2 * i), ()); TEST(!cbv->GetBit(2 * i + 1), ()); @@ -436,15 +436,15 @@ UNIT_TEST(CompressedBitVector_DenseLeaveFirstNBits) UNIT_TEST(CompressedBitVector_SparseLeaveFirstNBits) { vector setBits; - for (int p = 0; p < 20; ++p) + for (int p = 0; p < 10; ++p) setBits.push_back(static_cast(1) << p); auto cbv = coding::CompressedBitVectorBuilder::FromBitPositions(setBits); - TEST_EQUAL(cbv->PopCount(), 20, ()); + TEST_EQUAL(cbv->PopCount(), 10, ()); TEST_EQUAL(cbv->GetStorageStrategy(), coding::CompressedBitVector::StorageStrategy::Sparse, ()); cbv = cbv->LeaveFirstSetNBits(100); - TEST_EQUAL(cbv->PopCount(), 20, ()); - for (uint64_t bit = 0; bit < (1 << 20); ++bit) + TEST_EQUAL(cbv->PopCount(), 10, ()); + for (uint64_t bit = 0; bit < (1 << 10); ++bit) { if (bit != 0 && (bit & (bit - 1)) == 0) TEST(cbv->GetBit(bit), (bit)); @@ -452,11 +452,11 @@ UNIT_TEST(CompressedBitVector_SparseLeaveFirstNBits) TEST(!cbv->GetBit(bit), (bit)); } - cbv = cbv->LeaveFirstSetNBits(10); - TEST_EQUAL(cbv->PopCount(), 10, ()); - for (uint64_t bit = 0; bit < (1 << 20); ++bit) + cbv = cbv->LeaveFirstSetNBits(8); + TEST_EQUAL(cbv->PopCount(), 8, ()); + for (uint64_t bit = 0; bit < (1 << 10); ++bit) { - if (bit != 0 && (bit & (bit - 1)) == 0 && bit < (1 << 10)) + if (bit != 0 && (bit & (bit - 1)) == 0 && bit < (1 << 8)) TEST(cbv->GetBit(bit), (bit)); else TEST(!cbv->GetBit(bit), (bit)); @@ -464,6 +464,6 @@ UNIT_TEST(CompressedBitVector_SparseLeaveFirstNBits) cbv = cbv->LeaveFirstSetNBits(0); TEST_EQUAL(cbv->PopCount(), 0, ()); - for (uint64_t bit = 0; bit < (1 << 20); ++bit) + for (uint64_t bit = 0; bit < (1 << 10); ++bit) TEST(!cbv->GetBit(bit), (bit)); } diff --git a/coding/compressed_bit_vector.hpp b/coding/compressed_bit_vector.hpp index 6e01ee7056..7e51b363aa 100644 --- a/coding/compressed_bit_vector.hpp +++ b/coding/compressed_bit_vector.hpp @@ -58,6 +58,8 @@ public: // Would operator[] look better? virtual bool GetBit(uint64_t pos) const = 0; + // Returns a subset of the current bit vector with first + // min(PopCount(), |n|) set bits. virtual unique_ptr LeaveFirstSetNBits(uint64_t n) const = 0; // Returns the strategy used when storing this bit vector. diff --git a/search/v2/features_filter.cpp b/search/v2/features_filter.cpp index 1e2bce62ae..ffe474494e 100644 --- a/search/v2/features_filter.cpp +++ b/search/v2/features_filter.cpp @@ -43,15 +43,7 @@ unique_ptr ViewportFilter::Filter( auto result = coding::CompressedBitVector::Intersect(m_filter, cbv); if (!coding::CompressedBitVector::IsEmpty(result)) return result; - - uint64_t limit = std::min(static_cast(m_threshold), cbv.PopCount()); - vector positions; - for (uint64_t pos = 0; positions.size() != limit; ++pos) - { - if (cbv.GetBit(pos)) - positions.push_back(pos); - } - return coding::CompressedBitVectorBuilder::FromBitPositions(move(positions)); + return cbv.LeaveFirstSetNBits(m_threshold); } } // namespace v2 } // namespace search diff --git a/search/v2/features_filter.hpp b/search/v2/features_filter.hpp index 8ccb9b044e..f84ca3a7bf 100644 --- a/search/v2/features_filter.hpp +++ b/search/v2/features_filter.hpp @@ -13,7 +13,7 @@ namespace v2 { // A lightweight filter of features. // -// NOTE: this class and it's subclasses *ARE* thread-safe. +// NOTE: this class and its subclasses *ARE* thread-safe. class FeaturesFilter { public: @@ -31,8 +31,8 @@ protected: uint32_t const m_threshold; }; -// Exact filter - leaves only features belonging to the set it had -// been constructed from. +// Exact filter - leaves only features belonging to the set it was +// constructed from. class LocalityFilter : public FeaturesFilter { public: @@ -44,10 +44,10 @@ public: }; // Fuzzy filter - tries to leave only features belonging to the set it -// had been constructed from, but if the result is empty, leaves at -// most first |threshold| features. This property is quite useful when -// there are no matching features in viewport but it's ok to process a -// limited number of features outside the viewport. +// was constructed from, but if the result is empty, leaves at most +// first |threshold| features instead. This property is quite useful +// when there are no matching features in viewport but it's ok to +// process a limited number of features outside the viewport. class ViewportFilter : public FeaturesFilter { public: diff --git a/search/v2/geocoder.cpp b/search/v2/geocoder.cpp index 8240d59ef1..8f142ebdad 100644 --- a/search/v2/geocoder.cpp +++ b/search/v2/geocoder.cpp @@ -1095,7 +1095,7 @@ void Geocoder::MatchPOIsAndBuildings(size_t curToken) bool const looksLikeHouseNumber = feature::IsHouseNumber(m_layers.back().m_subQuery); - if (coding::CompressedBitVector::IsEmpty(features.Get()) && !looksLikeHouseNumber) + if (features.IsEmpty() && !looksLikeHouseNumber) break; if (n == 1) diff --git a/search/v2/geocoder.hpp b/search/v2/geocoder.hpp index 1d1fd33c6f..18a92b6431 100644 --- a/search/v2/geocoder.hpp +++ b/search/v2/geocoder.hpp @@ -188,7 +188,7 @@ private: void MatchViewportAndPosition(); // Tries to do geocoding in a limited scope, assuming that knowledge - // about high-level features, like cities or countries, is somehow + // about high-level features, like cities or countries, is // incorporated into |filter|. void LimitedSearch(FeaturesFilter const & filter);