From 7e21339eacb8154fee47c51baa60ee2c716f49e6 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Fri, 3 Apr 2015 14:23:08 +0300 Subject: [PATCH] Review fixes. --- base/observer_list.hpp | 2 +- base/timer.cpp | 1 + indexer/index.cpp | 2 +- indexer/index.hpp | 39 +++++++++++++------------- indexer/indexer_tests/index_test.cpp | 10 +++---- indexer/indexer_tests/mwm_set_test.cpp | 4 +-- indexer/mwm_set.cpp | 4 +-- indexer/mwm_set.hpp | 24 ++++++++-------- map/feature_vec_model.hpp | 29 ++++++++++--------- map/framework.hpp | 2 +- 10 files changed, 60 insertions(+), 57 deletions(-) diff --git a/base/observer_list.hpp b/base/observer_list.hpp index 6599afbe91..8c51708f5a 100644 --- a/base/observer_list.hpp +++ b/base/observer_list.hpp @@ -27,7 +27,7 @@ public: return true; } - bool Remove(TObserver & observer) + bool Remove(TObserver const & observer) { lock_guard lock(m_observersLock); auto it = find(m_observers.begin(), m_observers.end(), &observer); diff --git a/base/timer.cpp b/base/timer.cpp index 51568af311..accb7f1ea0 100644 --- a/base/timer.cpp +++ b/base/timer.cpp @@ -68,6 +68,7 @@ uint32_t TodayAsYYMMDD() { time_t rawTime = time(NULL); tm * pTm = gmtime(&rawTime); + CHECK(pTm, ("Can't get current date.")); return GenerateTimestamp(pTm->tm_year, pTm->tm_mon, pTm->tm_mday); } diff --git a/indexer/index.cpp b/indexer/index.cpp index 5566c80a56..1869734465 100644 --- a/indexer/index.cpp +++ b/indexer/index.cpp @@ -30,7 +30,7 @@ string MwmValue::GetFileName() const // Index implementation ////////////////////////////////////////////////////////////////////////////////// -bool Index::GetVersion(string const & name, MwmInfo & info) +bool Index::GetVersion(string const & name, MwmInfo & info) const { MwmValue value(name); diff --git a/indexer/index.hpp b/indexer/index.hpp index c8356a10b1..be929745fe 100644 --- a/indexer/index.hpp +++ b/indexer/index.hpp @@ -37,7 +37,7 @@ class Index : public MwmSet { protected: // MwmSet overrides: - bool GetVersion(string const & name, MwmInfo & info) override; + bool GetVersion(string const & name, MwmInfo & info) const override; MwmValue * CreateValue(string const & name) const override; void UpdateMwmInfo(MwmId id) override; @@ -61,50 +61,51 @@ public: /// Called when a map is registered for a first time. virtual void OnMapRegistered(string const & file) {} - /// Called when update for a map is downloaded. + /// Called when an update for a map is downloaded. virtual void OnMapUpdateIsReady(string const & file) {} - /// Called when update for a map is applied. + /// Called when an update for a map is applied. virtual void OnMapUpdated(string const & file) {} - /// Called when map is deleted. + /// Called when a map is deleted. virtual void OnMapDeleted(string const & file) {} }; Index(); ~Index(); - /// Registers new map. + /// Registers a new map. /// - /// \return A pair of MwmLock and a flag. MwmLock is locked iff map - /// with fileName was created or already exists. Flag is - /// set when a new map was registered. Thus, there are - /// three main cases: - /// * map already exists - returns active lock and unset flag - /// * a new map was registered - returns active lock and set flag - /// * can't register new map - returns inactive lock and unset flag + /// \return A pair of an MwmLock and a flag. MwmLock is locked iff the + /// map with fileName was created or already exists. Flag + /// is set when the map was registered for a first + /// time. Thus, there are three main cases: + /// + /// * the map already exists - returns active lock and unset flag + /// * the map was already registered - returns active lock and set flag + /// * the map can't be registered - returns inactive lock and unset flag WARN_UNUSED_RESULT pair RegisterMap(string const & fileName); - /// Replaces map file corresponding to fileName with a new one, when + /// Replaces a map file corresponding to fileName with a new one, when /// it's possible - no clients of the map file. Otherwise, update /// will be delayed. /// - /// \return * map file have been updated - returns active lock and + /// \return * the map file have been updated - returns active lock and /// UPDATE_STATUS_OK - /// * update is delayed because map is busy - returns active lock and + /// * update is delayed because the map is busy - returns active lock and /// UPDATE_STATUS_UPDATE_DELAYED - /// * file isn't suitable for update - returns inactive lock and + /// * the file isn't suitable for update - returns inactive lock and /// UPDATE_STATUS_BAD_FILE WARN_UNUSED_RESULT pair UpdateMap(string const & fileName); - /// Deletes map both from file system and internal tables, also, - /// deletes all files related to the map. If map was successfully + /// Deletes a map both from the file system and internal tables, also, + /// deletes all files related to the map. If the map was successfully /// deleted, notifies observers. /// /// \param fileName A fileName denoting the map to be deleted, may /// be a full path or a short path relative to /// executable's directories. - //// \return True if map was successfully deleted. + //// \return True if the map was successfully deleted. bool DeleteMap(string const & fileName); bool AddObserver(Observer & observer); diff --git a/indexer/indexer_tests/index_test.cpp b/indexer/indexer_tests/index_test.cpp index aabe4c7427..288eddaa11 100644 --- a/indexer/indexer_tests/index_test.cpp +++ b/indexer/indexer_tests/index_test.cpp @@ -100,7 +100,7 @@ UNIT_TEST(Index_MwmStatusNotifications) TEST_EQUAL(0, observer.map_registered_calls(), ()); - // Check that observers are triggered after map registration. + // Checks that observers are triggered after map registration. { pair p = index.RegisterMap(testMapName); TEST(p.first.IsLocked(), ()); @@ -108,7 +108,7 @@ UNIT_TEST(Index_MwmStatusNotifications) TEST_EQUAL(1, observer.map_registered_calls(), ()); } - // Check that map can't registered twice and observers aren't + // Checks that map can't registered twice and observers aren't // triggered. { pair p = index.RegisterMap(testMapName); @@ -120,7 +120,7 @@ UNIT_TEST(Index_MwmStatusNotifications) TEST(my::CopyFileX(testMapPath, testMapUpdatePath), ()); MY_SCOPE_GUARD(testMapUpdateGuard, bind(&CheckedDeleteFile, testMapUpdatePath)); - // Check that observers are notified when map is deleted. + // Checks that observers are notified when map is deleted. { TEST_EQUAL(0, observer.map_update_is_ready_calls(), ()); TEST_EQUAL(0, observer.map_updated_calls(), ()); @@ -131,7 +131,7 @@ UNIT_TEST(Index_MwmStatusNotifications) TEST_EQUAL(1, observer.map_updated_calls(), ()); } - // Try to delete map in presence of active lock. Map should be + // Tries to delete map in presence of active lock. Map should be // marked "to be removed" but can't be deleted. { MwmSet::MwmLock lock(index, testMapName); @@ -141,7 +141,7 @@ UNIT_TEST(Index_MwmStatusNotifications) TEST_EQUAL(0, observer.map_deleted_calls(), ()); } - // Check that observers are notified when lock is destroyed. + // Checks that observers are notified when all locks are destroyed. TEST_EQUAL(1, observer.map_deleted_calls(), ()); index.RemoveObserver(observer); diff --git a/indexer/indexer_tests/mwm_set_test.cpp b/indexer/indexer_tests/mwm_set_test.cpp index b167b1948e..7b7fc72ce7 100644 --- a/indexer/indexer_tests/mwm_set_test.cpp +++ b/indexer/indexer_tests/mwm_set_test.cpp @@ -11,7 +11,7 @@ namespace class TestMwmSet : public MwmSet { protected: - virtual bool GetVersion(string const & path, MwmInfo & info) + bool GetVersion(string const & path, MwmInfo & info) const override { int n = path[0] - '0'; info.m_maxScale = n; @@ -20,7 +20,7 @@ namespace return true; } - virtual MwmValue * CreateValue(string const &) const + MwmValue * CreateValue(string const &) const override { return new MwmValue(); } diff --git a/indexer/mwm_set.cpp b/indexer/mwm_set.cpp index 608100b3fe..a56f0d5599 100644 --- a/indexer/mwm_set.cpp +++ b/indexer/mwm_set.cpp @@ -259,11 +259,11 @@ MwmSet::MwmValueBase * MwmSet::LockValueImpl(MwmId id) { ASSERT_LESS(id, m_info.size(), ()); if (id >= m_info.size()) - return NULL; + return nullptr; UpdateMwmInfo(id); if (!m_info[id].IsUpToDate()) - return NULL; + return nullptr; ++m_info[id].m_lockCount; diff --git a/indexer/mwm_set.hpp b/indexer/mwm_set.hpp index 217c95fbce..637ffff124 100644 --- a/indexer/mwm_set.hpp +++ b/indexer/mwm_set.hpp @@ -105,17 +105,16 @@ public: NONCOPYABLE(MwmLock); }; - /// Registers new map in the set. + /// Registers a new map. /// - /// \param fileName File name (without full path) of country. + /// \return A pair of an MwmLock and a flag. MwmLock is locked iff the + /// map with fileName was created or already exists. Flag + /// is set when the map was registered for a first + /// time. Thus, there are three main cases: /// - /// \return A pair of MwmLock and a flag. MwmLock is locked iff map - /// with fileName was created or already exists. Flag is - /// set when a new map was registered. Thus, there are - /// three main cases: - /// * map already exists - returns active lock and unset flag - /// * a new map was registered - returns active lock and set flag - /// * can't register new map - returns inactive lock and unset flag + /// * the map already exists - returns active lock and unset flag + /// * the map was already registered - returns active lock and set flag + /// * the map can't be registered - returns inactive lock and unset flag //@{ protected: WARN_UNUSED_RESULT pair RegisterImpl(string const & fileName); @@ -127,9 +126,10 @@ public: /// @name Remove mwm. //@{ protected: - /// Deregisters map from the set when it' possible. Note that + /// Deregisters a map from the set when it's possible. Note that an /// underlying file is not deleted. - /// @return true - map is free; false - map is busy + /// + /// @return true when the map was deregistered. //@{ bool DeregisterImpl(MwmId id); bool DeregisterImpl(string const & fileName); @@ -157,7 +157,7 @@ public: protected: /// @return True when it's possible to get file format version - in /// this case version is set to the file format version. - virtual bool GetVersion(string const & name, MwmInfo & info) = 0; + virtual bool GetVersion(string const & name, MwmInfo & info) const = 0; virtual MwmValueBase * CreateValue(string const & name) const = 0; void Cleanup(); diff --git a/map/feature_vec_model.hpp b/map/feature_vec_model.hpp index e40830cb1a..c75528725f 100644 --- a/map/feature_vec_model.hpp +++ b/map/feature_vec_model.hpp @@ -32,18 +32,19 @@ namespace model public: void InitClassificator(); - /// Registers new map. + /// Registers a new map. /// - /// \return A pair of MwmLock and a flag. MwmLock is locked iff map - /// with fileName was created or already exists. Flag is - /// set when a new map was registered. Thus, there are - /// three main cases: - /// * map already exists - returns active lock and unset flag - /// * a new map was registered - returns active lock and set flag - /// * can't register new map - returns inactive lock and unset flag + /// \return A pair of an MwmLock and a flag. MwmLock is locked iff the + /// map with fileName was created or already exists. Flag + /// is set when the map was registered for a first + /// time. Thus, there are three main cases: + /// + /// * the map already exists - returns active lock and unset flag + /// * the map was already registered - returns active lock and set flag + /// * the map can't be registered - returns inactive lock and unset flag WARN_UNUSED_RESULT pair RegisterMap(string const & file); - /// Deregisters map denoted by file from internal records. + /// Deregisters a map denoted by file from internal records. void DeregisterMap(string const & file); /// Deregisters all registered maps. @@ -51,18 +52,18 @@ namespace model /// Deletes all files related to map denoted by file. /// - /// \return True if map was successfully deleted. + /// \return True if a map was successfully deleted. bool DeleteMap(string const & file); - /// Replaces map file corresponding to fileName with a new one, when + /// Replaces a map file corresponding to fileName with a new one, when /// it's possible - no clients of the map file. Otherwise, update /// will be delayed. /// - /// \return * map file have been updated - returns active lock and + /// \return * the map file have been updated - returns active lock and /// UPDATE_STATUS_OK - /// * update is delayed because map is busy - returns active lock and + /// * update is delayed because the map is busy - returns active lock and /// UPDATE_STATUS_UPDATE_DELAYED - /// * file isn't suitable for update - returns inactive lock and + /// * the file isn't suitable for update - returns inactive lock and /// UPDATE_STATUS_BAD_FILE WARN_UNUSED_RESULT pair UpdateMap(string const & file); //@} diff --git a/map/framework.hpp b/map/framework.hpp index 7d35d231b3..3bb147c5f4 100644 --- a/map/framework.hpp +++ b/map/framework.hpp @@ -170,7 +170,7 @@ public: /// Deregisters all registered map files. void DeregisterAllMaps(); - /// Registers local map file in internal indexes. + /// Registers a local map file in internal indexes. /// /// @return True and inner mwm data version from header in version /// or false in case of errors.