From 545f8318996cea4ab9136fb28e38c1dc5012c148 Mon Sep 17 00:00:00 2001 From: Arsentiy Milchakov Date: Mon, 31 Jul 2017 17:08:17 +0300 Subject: review fixes --- local_ads/campaign.hpp | 15 ++- local_ads/campaign_serialization.cpp | 110 +++++++++++++++------ local_ads/campaign_serialization.hpp | 8 +- .../campaign_serialization_test.cpp | 20 ++-- local_ads/pylocal_ads/bindings_test.py | 31 +++--- map/local_ads_manager.cpp | 2 +- 6 files changed, 118 insertions(+), 68 deletions(-) diff --git a/local_ads/campaign.hpp b/local_ads/campaign.hpp index 5eb795ef7f..52815a6429 100644 --- a/local_ads/campaign.hpp +++ b/local_ads/campaign.hpp @@ -29,15 +29,22 @@ struct Campaign std::string GetIconName() const { return IconsInfo::Instance().GetIcon(m_iconId); } uint32_t m_featureId = 0; uint16_t m_iconId = 0; + // Supported values range: 0-255. In case when accurate value is more than 255, we expect updated + // data will be received during this time interval. For ex. accurate value is 365, in this case + // first 110 days this field will store value 255. uint8_t m_daysBeforeExpired = 0; - uint8_t m_minZoomLevel = 16; // supported values range: 10-17 - uint8_t m_priority = 0; // supported values range: 0-7 + // Supported values range: 10-17. + uint8_t m_minZoomLevel = 16; + // Supported values range: 0-7 + uint8_t m_priority = 0; }; inline bool operator==(Campaign const & a, Campaign const & b) { - return a.m_featureId == b.m_featureId && a.m_iconId == b.m_iconId && - a.m_daysBeforeExpired == b.m_daysBeforeExpired && a.m_minZoomLevel == b.m_minZoomLevel && + return a.m_featureId == b.m_featureId && + a.m_iconId == b.m_iconId && + a.m_daysBeforeExpired == b.m_daysBeforeExpired && + a.m_minZoomLevel == b.m_minZoomLevel && a.m_priority == b.m_priority; } } // namespace local_ads diff --git a/local_ads/campaign_serialization.cpp b/local_ads/campaign_serialization.cpp index 4b64e1a87d..7c20bf7042 100644 --- a/local_ads/campaign_serialization.cpp +++ b/local_ads/campaign_serialization.cpp @@ -13,8 +13,9 @@ namespace { using namespace local_ads; -auto const kHalfByteShift = 0x4; -auto const kLowerMask = 0xF; +auto const kHalfByteShift = CHAR_BIT / 2; +auto const kHalfByteMaxValue = 15; +auto const kLowerMask = 0x0F; auto const kUpperMask = 0xF0; auto const kMinZoomLevel = 10; auto const kMaxZoomLevel = 17; @@ -61,10 +62,27 @@ std::vector ReadData(ByteStream & s, size_t chunksNumber) return result; } + +std::vector SerializeV1(std::vector const & campaigns) +{ + std::vector buff; + PushBackByteSink dst(buff); + Write(dst, Version::V1); + Write(dst, campaigns.size()); + for (auto const & c : campaigns) + WriteVarUint(dst, c.m_featureId); + for (auto const & c : campaigns) + WriteVarUint(dst, c.m_iconId); + for (auto const & c : campaigns) + Write(dst, c.m_daysBeforeExpired); + + return buff; +} + std::vector DeserializeV1(std::vector const & bytes) { ArrayByteSource src(bytes.data()); - CHECK_EQUAL(Read(src), Version::v1, ()); + CHECK_EQUAL(Read(src), Version::V1, ()); auto const chunksNumber = Read(src); auto const featureIds = ReadData(src, chunksNumber); @@ -88,10 +106,52 @@ uint8_t ZoomIndex(uint8_t zoomValue) { return zoomValue - kMinZoomLevel; } uint8_t ZoomValue(uint8_t zoomIndex) { return zoomIndex + kMinZoomLevel; } +uint8_t PackZoomAndPriority(uint8_t minZoomLevel, uint8_t priority) +{ + ASSERT_GREATER_OR_EQUAL(minZoomLevel, kMinZoomLevel, ("Unsupported zoom level")); + ASSERT_LESS_OR_EQUAL(minZoomLevel, kMaxZoomLevel, ("Unsupported zoom level")); + ASSERT_LESS_OR_EQUAL(priority, kMaxPriority, ("Unsupported priority value")); + + auto const zoomIndex = ZoomIndex(minZoomLevel); + ASSERT_LESS_OR_EQUAL(zoomIndex, kHalfByteMaxValue, ()); + ASSERT_LESS_OR_EQUAL(priority, kHalfByteMaxValue, ()); + + // Pack zoom and priority into single byte. + return (zoomIndex & kLowerMask) | ((priority << kHalfByteShift) & kUpperMask); +} + +uint8_t UnpackZoom(uint8_t src) +{ + return ZoomValue(src & kLowerMask); +} + +uint8_t UnpackPriority(uint8_t src) +{ + return (src >> kHalfByteShift) & kLowerMask; +} + +std::vector SerializeV2(std::vector const & campaigns) +{ + std::vector buff; + PushBackByteSink dst(buff); + Write(dst, Version::V2); + Write(dst, campaigns.size()); + for (auto const & c : campaigns) + WriteVarUint(dst, c.m_featureId); + for (auto const & c : campaigns) + WriteVarUint(dst, c.m_iconId); + for (auto const & c : campaigns) + Write(dst, c.m_daysBeforeExpired); + for (auto const & c : campaigns) + Write(dst, PackZoomAndPriority(c.m_minZoomLevel, c.m_priority)); + + return buff; +} + std::vector DeserializeV2(std::vector const & bytes) { ArrayByteSource src(bytes.data()); - CHECK_EQUAL(Read(src), Version::v2, ()); + CHECK_EQUAL(Read(src), Version::V2, ()); auto const chunksNumber = Read(src); auto const featureIds = ReadData(src, chunksNumber); @@ -109,8 +169,8 @@ std::vector DeserializeV2(std::vector const & bytes) for (size_t i = 0; i < chunksNumber; ++i) { campaigns.emplace_back(featureIds[i], icons[i], expirations[i], - ZoomValue(zoomAndPriority[i] & kLowerMask), - (zoomAndPriority[i] >> kHalfByteShift) & kLowerMask); + UnpackZoom(zoomAndPriority[i]), + UnpackPriority(zoomAndPriority[i])); ASSERT_GREATER_OR_EQUAL(campaigns.back().m_minZoomLevel, kMinZoomLevel, ("Unsupported zoom level")); @@ -126,32 +186,19 @@ namespace local_ads { std::vector Serialize(std::vector const & campaigns, Version const version) { - std::vector buff; - PushBackByteSink dst(buff); - Write(dst, version); - Write(dst, campaigns.size()); - for (auto const & c : campaigns) - WriteVarUint(dst, c.m_featureId); - for (auto const & c : campaigns) - WriteVarUint(dst, c.m_iconId); - for (auto const & c : campaigns) - Write(dst, c.m_daysBeforeExpired); - for (auto const & c : campaigns) + switch (version) { - ASSERT_GREATER_OR_EQUAL(c.m_minZoomLevel, kMinZoomLevel, ("Unsupported zoom level")); - ASSERT_LESS_OR_EQUAL(c.m_minZoomLevel, kMaxZoomLevel, ("Unsupported zoom level")); - ASSERT_LESS_OR_EQUAL(c.m_priority, kMaxPriority, ("Unsupported priority value")); - - Write(dst, static_cast((ZoomIndex(c.m_minZoomLevel) & kLowerMask) | - ((c.m_priority << kHalfByteShift) & kUpperMask))); + case Version::V1: return SerializeV1(campaigns); + case Version::V2: return SerializeV2(campaigns); + default: ASSERT(false, ("Unknown version")); } - return buff; + return {}; } std::vector Serialize(std::vector const & campaigns) { - return Serialize(campaigns, Version::latest); + return Serialize(campaigns, Version::Latest); } std::vector Deserialize(std::vector const & bytes) @@ -161,9 +208,9 @@ std::vector Deserialize(std::vector const & bytes) switch (version) { - case Version::v1: return DeserializeV1(bytes); - case Version::v2: return DeserializeV2(bytes); - default: ASSERT(false, ("Unknown version type")); + case Version::V1: return DeserializeV1(bytes); + case Version::V2: return DeserializeV2(bytes); + default: ASSERT(false, ("Unknown version")); } return {}; @@ -175,9 +222,10 @@ std::string DebugPrint(local_ads::Version version) switch (version) { - case Version::unknown: return "Unknown"; - case Version::v1: return "Version 1"; - case Version::v2: return "Version 2"; + case Version::Unknown: return "Unknown"; + case Version::V1: return "Version 1"; + case Version::V2: return "Version 2"; + default: ASSERT(false, ("Unknown version")); } } } // namespace local_ads diff --git a/local_ads/campaign_serialization.hpp b/local_ads/campaign_serialization.hpp index 167c0259a0..2434db8367 100644 --- a/local_ads/campaign_serialization.hpp +++ b/local_ads/campaign_serialization.hpp @@ -9,14 +9,14 @@ namespace local_ads { enum class Version { - unknown = -1, + Unknown = -1, // March 2017 (store feature ids and icon ids as varints, use one byte for days before // expiration). - v1 = 0, + V1 = 0, // August 2017 (store zoom level and priority as 0-7 values in one byte). - v2 = 1, + V2 = 1, - latest = v2 + Latest = V2 }; std::vector Serialize(std::vector const & campaigns, Version const version); std::vector Serialize(std::vector const & campaigns); diff --git a/local_ads/local_ads_tests/campaign_serialization_test.cpp b/local_ads/local_ads_tests/campaign_serialization_test.cpp index 40a69017bd..4cec8f9ab0 100644 --- a/local_ads/local_ads_tests/campaign_serialization_test.cpp +++ b/local_ads/local_ads_tests/campaign_serialization_test.cpp @@ -36,8 +36,8 @@ std::vector GenerateCampaignsV1(size_t number) std::vector GenerateCampaignsV2(size_t number) { - std::random_device rd; - std::mt19937 gen(rd()); + int kSeed = 42; + std::mt19937 gen(kSeed); std::uniform_int_distribution<> featureIds(1, 600000); std::uniform_int_distribution<> icons(1, 4096); std::uniform_int_distribution<> expirationDays(1, 30); @@ -64,19 +64,19 @@ UNIT_TEST(Serialization_Smoke) {10, 10, 10}, {1000, 100, 20}, {120003, 456, 15} - }, Version::v1), ()); + }, Version::V1), ()); TEST(TestSerialization({ {10, 10, 10, 10, 0}, {1000, 100, 20, 17, 7}, {120003, 456, 15, 13, 6} - }, Version::v2), ()); + }, Version::V2), ()); - TEST(TestSerialization(GenerateCampaignsV1(100), Version::v1), ()); - TEST(TestSerialization(GenerateCampaignsV1(1000), Version::v1), ()); - TEST(TestSerialization(GenerateCampaignsV1(10000), Version::v1), ()); + TEST(TestSerialization(GenerateCampaignsV1(100), Version::V1), ()); + TEST(TestSerialization(GenerateCampaignsV1(1000), Version::V1), ()); + TEST(TestSerialization(GenerateCampaignsV1(10000), Version::V1), ()); - TEST(TestSerialization(GenerateCampaignsV2(100), Version::v2), ()); - TEST(TestSerialization(GenerateCampaignsV2(1000), Version::v2), ()); - TEST(TestSerialization(GenerateCampaignsV2(10000), Version::v2), ()); + TEST(TestSerialization(GenerateCampaignsV2(100), Version::V2), ()); + TEST(TestSerialization(GenerateCampaignsV2(1000), Version::V2), ()); + TEST(TestSerialization(GenerateCampaignsV2(10000), Version::V2), ()); } diff --git a/local_ads/pylocal_ads/bindings_test.py b/local_ads/pylocal_ads/bindings_test.py index 4decdc314a..f183a99120 100644 --- a/local_ads/pylocal_ads/bindings_test.py +++ b/local_ads/pylocal_ads/bindings_test.py @@ -1,27 +1,22 @@ -from pylocal_ads import (Campaign, serialize, deserialize) +import unittest +from pylocal_ads import (Campaign, serialize, deserialize) -def smoke(): - campaigns = [ - Campaign(10, 10, 10, 10, 0), - Campaign(1000, 100, 20, 17, 7), - Campaign(120003, 456, 15, 13, 6) - ] - serialized = serialize(campaigns) - result = deserialize(serialized) +class PyLocalAdsTest(unittest.TestCase): - if campaigns.sort() == result.sort(): - return True + def test_smoke(self): + campaigns = [ + Campaign(10, 10, 10, 10, 0), + Campaign(1000, 100, 20, 17, 7), + Campaign(120003, 456, 15, 13, 6) + ] - return False + serialized = serialize(campaigns) + result = deserialize(serialized) + self.assertEqual(campaigns.sort(), result.sort()) -def main(): - if smoke(): - print "Smoke OK" - else: - print "Smoke FAIL" if __name__ == "__main__": - main() + unittest.main() diff --git a/map/local_ads_manager.cpp b/map/local_ads_manager.cpp index fd16295845..59c7dfe6da 100644 --- a/map/local_ads_manager.cpp +++ b/map/local_ads_manager.cpp @@ -79,7 +79,7 @@ std::string MakeCampaignDownloadingURL(MwmSet::MwmId const & mwmId) return {}; std::ostringstream ss; - auto const campaignDataVersion = static_cast(local_ads::Version::latest); + auto const campaignDataVersion = static_cast(local_ads::Version::Latest); ss << kServerUrl << "/" << campaignDataVersion << "/" << mwmId.GetInfo()->GetVersion() << "/" -- cgit v1.2.3