From 68555fdfffab698675cf6422c38a7b7633964dd6 Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Thu, 18 Feb 2016 09:02:03 +0300 Subject: [PATCH] [new downloader] Review fixes. --- storage/simple_tree.hpp | 12 ++++++++---- storage/storage.cpp | 18 +++++++++--------- storage/storage.hpp | 18 +++++++++--------- storage/storage_tests/simple_tree_test.cpp | 2 +- storage/storage_tests/storage_tests.cpp | 8 ++++---- 5 files changed, 31 insertions(+), 27 deletions(-) diff --git a/storage/simple_tree.hpp b/storage/simple_tree.hpp index b8e800b0a7..aa0db4a957 100644 --- a/storage/simple_tree.hpp +++ b/storage/simple_tree.hpp @@ -32,11 +32,14 @@ public: return m_value; } + /// \brief Reserves child size vector. This method should be called once before filling + /// children vector with correct |n| size to prevent m_children relocation while filling. void ReserveAtDepth(int level, size_t n) { SimpleTree * node = this; while (level-- > 0 && !node->m_children.empty()) node = &node->m_children.back(); + ASSERT_EQUAL(level, -1, ()); return node->Reserve(n); } @@ -57,6 +60,7 @@ public: SimpleTree * node = this; while (level-- > 0 && !node->m_children.empty()) node = &node->m_children.back(); + ASSERT_EQUAL(level, -1, ()); return node->Add(value); } @@ -197,20 +201,20 @@ public: } template - void ForEachParentExceptForTheRoot(TFunctor && f) + void ForEachAncestorExceptForTheRoot(TFunctor && f) { if (m_parent == nullptr || m_parent->m_parent == nullptr) return; f(*m_parent); - m_parent->ForEachParentExceptForTheRoot(f); + m_parent->ForEachAncestorExceptForTheRoot(f); } template - void ForEachParentExceptForTheRoot(TFunctor && f) const + void ForEachAncestorExceptForTheRoot(TFunctor && f) const { if (m_parent == nullptr || m_parent->m_parent == nullptr) return; f(*m_parent); - m_parent->ForEachParentExceptForTheRoot(f); + m_parent->ForEachAncestorExceptForTheRoot(f); } }; diff --git a/storage/storage.cpp b/storage/storage.cpp index c88c5483fa..5c4a9a36d3 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -88,8 +88,8 @@ TCountriesContainer const & LeafNodeFromCountryId(TCountriesContainer const & ro void HashFromQueue(Storage::TQueue const & queue, TCountriesUnorderedSet & hashQueue) { hashQueue.reserve(queue.size()); - for (auto const & queuedCountry : queue) - hashQueue.insert(queuedCountry.GetCountryId()); + for (auto const & country : queue) + hashQueue.insert(country.GetCountryId()); } } // namespace @@ -687,7 +687,7 @@ void Storage::ReportProgressForHierarchy(TCountryId const & countryId, // This lambda will be called for every parent of countryId (except for the root) // to calculate and report parent's progress. - auto forEachParentExceptForTheRoot = [this, &leafProgress, &hashQueue, &countryId] + auto calcProgress = [this, &leafProgress, &hashQueue, &countryId] (TCountryId const & parentId, TCountriesVec const & descendants) { pair localAndRemoteBytes = CalculateProgress(descendants, @@ -695,7 +695,7 @@ void Storage::ReportProgressForHierarchy(TCountryId const & countryId, ReportProgress(parentId, localAndRemoteBytes); }; - ForEachParentExceptForTheRoot(countryId, forEachParentExceptForTheRoot); + ForEachAncestorExceptForTheRoot(countryId, calcProgress); } void Storage::OnServerListDownloaded(vector const & urls) @@ -1222,7 +1222,7 @@ Status Storage::NodeStatus(TCountriesContainer const & node) const set const kDownloadingInProgressSet = { Status::EDownloading, Status::EInQueue }; set const kUsersAttentionNeededSet = { Status::EDownloadFailed, Status::EOutOfMemFailed, Status::EUnknown, Status::EOnDiskOutOfDate }; - // The other statuses shows that everything is ok (kEverythingsOk). + // The other statuses say that everything is ok (kEverythingsOk). if (result == kDownloadingInProgress || nodeInSubtree.ChildrenCount() != 0) return; @@ -1274,7 +1274,7 @@ void Storage::GetNodeAttrs(TCountryId const & countryId, NodeAttrs & nodeAttrs) { descendants.push_back(d.Value().Name()); }); - TCountryId const downloadingMwm = IsDownloadInProgress() ? GetCurrentDownloadingCountryId() + TCountryId const & downloadingMwm = IsDownloadInProgress() ? GetCurrentDownloadingCountryId() : kInvalidCountryId; MapFilesDownloader::TProgress downloadingMwmProgress = m_downloader->IsIdle() ? make_pair(0LL, 0LL) @@ -1316,14 +1316,14 @@ void Storage::DoClickOnDownloadMap(TCountryId const & countryId) } pair Storage::CalculateProgress(TCountriesVec const & descendants, - TCountryId const & downlaodingMwm, + TCountryId const & downloadingMwm, pair const & downloadingMwmProgress, TCountriesUnorderedSet const & hashQueue) const { pair localAndRemoteBytes = make_pair(0, 0); for (auto const & d : descendants) { - if (d == downlaodingMwm) + if (d == downloadingMwm) continue; if (hashQueue.count(d) != 0) @@ -1339,7 +1339,7 @@ pair Storage::CalculateProgress(TCountriesVec const & descenda localAndRemoteBytes.second += localCountryFile.GetRemoteSize(MapOptions::Map); } } - if (downlaodingMwm != kInvalidCountryId) + if (downloadingMwm != kInvalidCountryId) { // Downloading is in progress right now. localAndRemoteBytes.first += downloadingMwmProgress.first; diff --git a/storage/storage.hpp b/storage/storage.hpp index d53a255b08..99f825af3b 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -245,7 +245,7 @@ public: TOnStatusChangedCallback m_onStatusChanged; }; - /// \brief Returns root country id of the county tree. + /// \brief Returns root country id of the country tree. TCountryId const GetRootId() const; /// \param childrenId is filled with children node ids by a parent. For example GetChildren(GetRootId()) @@ -325,7 +325,7 @@ public: template void ForEachInSubtree(TCountryId const & root, ToDo && toDo) const; template - void ForEachParentExceptForTheRoot(TCountryId const & childId, ToDo && toDo) const; + void ForEachAncestorExceptForTheRoot(TCountryId const & childId, ToDo && toDo) const; /// \brief Subscribe on change status callback. /// \returns a unique index of added status callback structure. @@ -508,15 +508,15 @@ private: /// Will be removed in future after refactoring. TCountriesVec FindAllIndexesByFile(TCountryId const & name) const; - /// Calculates progress of downloading for non-expandable nodes in county tree. + /// Calculates progress of downloading for non-expandable nodes in country tree. /// |descendants| All descendants of the parent node. - /// |downlaodingMwm| Downloading leaf node country id if any. If not, downlaodingMwm == kInvalidCountryId. - /// if downlaodingMwm != kInvalidCountryId |downloadingMwmProgress| is a progress of downloading + /// |downloadingMwm| Downloading leaf node country id if any. If not, downloadingMwm == kInvalidCountryId. + /// If downloadingMwm != kInvalidCountryId |downloadingMwmProgress| is a progress of downloading /// the leaf node in bytes. |downloadingMwmProgress.first| == number of downloaded bytes. /// |downloadingMwmProgress.second| == number of bytes in downloading files. /// |hashQueue| hash table made from |m_queue|. pair CalculateProgress(TCountriesVec const & descendants, - TCountryId const & downlaodingMwm, + TCountryId const & downloadingMwm, pair const & downloadingMwmProgress, TCountriesUnorderedSet const & hashQueue) const; }; @@ -542,10 +542,10 @@ void Storage::ForEachInSubtree(TCountryId const & root, ToDo && toDo) const /// for each parent (including indirect/great parents) except for the main root of the tree. /// |descendantsCountryId| is a vector of country id of descendats of |parentId|. /// Note. In case of disputable territories several nodes with the same name may be -/// present in the country tree. In that case ForEachParentExceptForTheRoot calls +/// present in the country tree. In that case ForEachAncestorExceptForTheRoot calls /// |toDo| for parents of each branch in the country tree. template -void Storage::ForEachParentExceptForTheRoot(TCountryId const & childId, ToDo && toDo) const +void Storage::ForEachAncestorExceptForTheRoot(TCountryId const & childId, ToDo && toDo) const { vector const *> nodes; m_countries.Find(Country(childId), nodes); @@ -559,7 +559,7 @@ void Storage::ForEachParentExceptForTheRoot(TCountryId const & childId, ToDo && // may be more than one. It means |childId| is present in the country tree more than once. for (auto const & node : nodes) { - node->ForEachParentExceptForTheRoot([&toDo](TCountriesContainer const & countryContainer) + node->ForEachAncestorExceptForTheRoot([&toDo](TCountriesContainer const & countryContainer) { TCountriesVec descendantsCountryId; countryContainer.ForEachDescendant([&descendantsCountryId](TCountriesContainer const & countryContainer) diff --git a/storage/storage_tests/simple_tree_test.cpp b/storage/storage_tests/simple_tree_test.cpp index 2c33b5b0c8..697ab15509 100644 --- a/storage/storage_tests/simple_tree_test.cpp +++ b/storage/storage_tests/simple_tree_test.cpp @@ -56,7 +56,7 @@ UNIT_TEST(SimpleTree_Smoke) TEST_EQUAL(c2.count, 8, ()); Calculator c3; - tree.Child(4).Child(0).ForEachParentExceptForTheRoot(c3); + tree.Child(4).Child(0).ForEachAncestorExceptForTheRoot(c3); TEST_EQUAL(c3.count, 1, ()); tree.Clear(); diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index dab196c24f..1c030913ea 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -1306,7 +1306,7 @@ UNIT_TEST(StorageTest_ForEachInSubtree) TEST_EQUAL(leafVec, expectedLeafVec, ()); } -UNIT_TEST(StorageTest_ForEachParentExceptForTheRoot) +UNIT_TEST(StorageTest_ForEachAncestorExceptForTheRoot) { Storage storage(kSingleMwmCountriesTxt, make_unique()); @@ -1328,7 +1328,7 @@ UNIT_TEST(StorageTest_ForEachParentExceptForTheRoot) } TEST(false, ()); }; - storage.ForEachParentExceptForTheRoot("Disputable Territory", forEachParentDisputableTerritory); + storage.ForEachAncestorExceptForTheRoot("Disputable Territory", forEachParentDisputableTerritory); // One parent case. auto const forEachParentIndisputableTerritory @@ -1342,8 +1342,8 @@ UNIT_TEST(StorageTest_ForEachParentExceptForTheRoot) } TEST(false, ()); }; - storage.ForEachParentExceptForTheRoot("Indisputable Territory Of Country1", - forEachParentIndisputableTerritory); + storage.ForEachAncestorExceptForTheRoot("Indisputable Territory Of Country1", + forEachParentIndisputableTerritory); } UNIT_TEST(StorageTest_CalcLimitRect)