From 4b920ba9cf3d856f40a3afea1b3ef5a7b1554ddb Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Mon, 7 Nov 2016 18:27:26 +0300 Subject: [PATCH] Review fixes. --- generator/feature_generator.cpp | 4 ++-- generator/feature_generator.hpp | 2 ++ generator/feature_sorter.cpp | 4 +--- generator/generate_info.hpp | 7 +++--- generator/generator.pro | 1 - generator/generator_tests/generator_tests.pro | 1 - .../restriction_collector_test.cpp | 24 +++++++++---------- .../generator_tests/restriction_test.cpp | 12 +++++----- generator/generator_tool/generator_tool.cpp | 9 +++---- generator/osm_source.cpp | 9 ++++--- generator/restriction_collector.cpp | 15 ++++++------ generator/restriction_collector.hpp | 22 +++++++++-------- generator/restriction_dumper.cpp | 6 ++--- generator/restriction_dumper.hpp | 2 +- indexer/routing.cpp | 4 ++-- 15 files changed, 62 insertions(+), 60 deletions(-) diff --git a/generator/feature_generator.cpp b/generator/feature_generator.cpp index 2215033bad..4cca790729 100644 --- a/generator/feature_generator.cpp +++ b/generator/feature_generator.cpp @@ -107,9 +107,9 @@ uint32_t FeaturesCollector::operator()(FeatureBuilder1 const & fb) { FeatureBuilder1::TBuffer bytes; fb.Serialize(bytes); - (void)WriteFeatureBase(bytes, fb); + uint32_t const featureId = WriteFeatureBase(bytes, fb); CHECK_LESS(0, m_featureID, ()); - return m_featureID - 1; + return featureId; } FeaturesAndRawGeometryCollector::FeaturesAndRawGeometryCollector(string const & featuresFileName, diff --git a/generator/feature_generator.hpp b/generator/feature_generator.hpp index 4a5a941f9f..7a17cc0b2b 100644 --- a/generator/feature_generator.hpp +++ b/generator/feature_generator.hpp @@ -4,6 +4,7 @@ #include "coding/file_writer.hpp" +#include "std/numeric.hpp" #include "std/string.hpp" #include "std/vector.hpp" @@ -19,6 +20,7 @@ class FeaturesCollector uint32_t m_featureID = 0; protected: + static uint32_t constexpr kInvalidFeatureId = numeric_limits::max(); FileWriter m_datFile; m2::RectD m_bounds; diff --git a/generator/feature_sorter.cpp b/generator/feature_sorter.cpp index b82c81a389..8659b4d91e 100644 --- a/generator/feature_sorter.cpp +++ b/generator/feature_sorter.cpp @@ -27,8 +27,6 @@ #include "base/scope_guard.hpp" #include "base/string_utils.hpp" -#include "std/limits.hpp" - namespace { typedef pair CellAndOffsetT; @@ -522,7 +520,7 @@ namespace feature } } - uint32_t featureId = numeric_limits::max(); + uint32_t featureId = kInvalidFeatureId; if (fb.PreSerialize(holder.m_buffer)) { fb.Serialize(holder.m_buffer, m_header.GetDefCodingParams()); diff --git a/generator/generate_info.hpp b/generator/generate_info.hpp index 9733060fee..092143d39f 100644 --- a/generator/generate_info.hpp +++ b/generator/generate_info.hpp @@ -36,10 +36,11 @@ struct GenerateInfo // Current generated file name if --output option is defined. string m_fileName; - // File name with restriction in osm id terms. + // File name for a file with restrictions in osm id terms. string m_restrictions; - // File name with mapping from feature id to osm ids. - string m_featureId2OsmIds; + // File name for a file with mapping from features id to osm ids. + // Note. One feature id maps to one or several osm ids. + string m_featureIdToOsmIds; NodeStorageType m_nodeStorageType; OsmSourceType m_osmFileType; diff --git a/generator/generator.pro b/generator/generator.pro index 917696bed2..22c8fb01f7 100644 --- a/generator/generator.pro +++ b/generator/generator.pro @@ -92,4 +92,3 @@ HEADERS += \ unpack_mwm.hpp \ ways_merger.hpp \ world_map_generator.hpp \ - diff --git a/generator/generator_tests/generator_tests.pro b/generator/generator_tests/generator_tests.pro index 0224bfef04..b5b74540ff 100644 --- a/generator/generator_tests/generator_tests.pro +++ b/generator/generator_tests/generator_tests.pro @@ -38,4 +38,3 @@ SOURCES += \ tag_admixer_test.cpp \ tesselator_test.cpp \ triangles_tree_coding_test.cpp \ - diff --git a/generator/generator_tests/restriction_collector_test.cpp b/generator/generator_tests/restriction_collector_test.cpp index 769e237860..c0d537ec29 100644 --- a/generator/generator_tests/restriction_collector_test.cpp +++ b/generator/generator_tests/restriction_collector_test.cpp @@ -5,13 +5,13 @@ #include "indexer/routing.hpp" -#include "coding/file_name_utils.hpp" - #include "platform/platform_tests_support/scoped_dir.hpp" #include "platform/platform_tests_support/scoped_file.hpp" #include "platform/platform.hpp" +#include "coding/file_name_utils.hpp" + #include "base/stl_helpers.hpp" #include "std/string.hpp" @@ -27,7 +27,7 @@ string const kRestrictionTestDir = "test-restrictions"; UNIT_TEST(RestrictionTest_ValidCase) { - RestrictionCollector restrictionCollector("", ""); + RestrictionCollector restrictionCollector("" /* restrictionPath */, "" /* featureId2OsmIdsPath */); // Adding restrictions and feature ids to restrictionCollector in mixed order. restrictionCollector.AddRestriction(Restriction::Type::No, {1, 2} /* osmIds */); restrictionCollector.AddFeatureId(30 /* featureId */, {3} /* osmIds */); @@ -53,7 +53,7 @@ UNIT_TEST(RestrictionTest_ValidCase) UNIT_TEST(RestrictionTest_InvalidCase) { - RestrictionCollector restrictionCollector("", ""); + RestrictionCollector restrictionCollector("" /* restrictionPath */, "" /* featureId2OsmIdsPath */); restrictionCollector.AddFeatureId(0 /* featureId */, {0} /* osmIds */); restrictionCollector.AddRestriction(Restriction::Type::No, {0, 1} /* osmIds */); restrictionCollector.AddFeatureId(20 /* featureId */, {2} /* osmIds */); @@ -84,21 +84,21 @@ UNIT_TEST(RestrictionTest_ParseRestrictions) ScopedDir const scopedDir(kRestrictionTestDir); ScopedFile const scopedFile(kRestrictionPath, kRestrictionContent); - RestrictionCollector restrictionCollector("", ""); + RestrictionCollector restrictionCollector("" /* restrictionPath */, "" /* featureId2OsmIdsPath */); Platform const & platform = Platform(); TEST(restrictionCollector.ParseRestrictions( my::JoinFoldersToPath(platform.WritableDir(), kRestrictionPath)), ()); - RestrictionVec expectedRestrictions = {{Restriction::Type::No, 2}, - {Restriction::Type::Only, 2}, - {Restriction::Type::Only, 2}, - {Restriction::Type::No, 2}, - {Restriction::Type::No, 2}}; + RestrictionVec expectedRestrictions = {{Restriction::Type::No, 2 /* linkNumber */}, + {Restriction::Type::Only, 2 /* linkNumber */}, + {Restriction::Type::Only, 2 /* linkNumber */}, + {Restriction::Type::No, 2 /* linkNumber */}, + {Restriction::Type::No, 2 /* linkNumber */}}; TEST_EQUAL(restrictionCollector.m_restrictions, expectedRestrictions, ()); - vector> const expectedRestrictionIndex = { + vector> const expectedRestrictionIndex = { {1, {0, 0}}, {1, {0, 1}}, {0, {1, 0}}, {2, {1, 1}}, {2, {2, 0}}, {3, {2, 1}}, {38028428, {3, 0}}, {38028428, {3, 1}}, {4, {4, 0}}, {5, {4, 1}}}; TEST_EQUAL(restrictionCollector.m_restrictionIndex, expectedRestrictionIndex, ()); @@ -117,7 +117,7 @@ UNIT_TEST(RestrictionTest_ParseFeatureId2OsmIdsMapping) ScopedDir const scopedDir(kRestrictionTestDir); ScopedFile const scopedFile(kFeatureIdToOsmIdsPath, kFeatureIdToOsmIdsContent); - RestrictionCollector restrictionCollector("", ""); + RestrictionCollector restrictionCollector("" /* restrictionPath */, "" /* featureId2OsmIdsPath */); Platform const & platform = Platform(); restrictionCollector.ParseFeatureId2OsmIdsMapping( diff --git a/generator/generator_tests/restriction_test.cpp b/generator/generator_tests/restriction_test.cpp index 555adf2289..33dd618c80 100644 --- a/generator/generator_tests/restriction_test.cpp +++ b/generator/generator_tests/restriction_test.cpp @@ -25,9 +25,9 @@ using namespace feature; using namespace generator; +using namespace platform::tests_support; using namespace platform; using namespace routing; -using namespace platform::tests_support; namespace { @@ -50,10 +50,10 @@ void BuildEmptyMwm(LocalCountryFile & country) void TestRestrictionBuilding(string const & restrictionContent, string const & mappingContent) { Platform & platform = GetPlatform(); - string const writeableDir = platform.WritableDir(); + string const writableDir = platform.WritableDir(); // Building empty mwm. - LocalCountryFile country(my::JoinFoldersToPath(writeableDir, kTestDir), CountryFile(kTestMwm), 1); + LocalCountryFile country(my::JoinFoldersToPath(writableDir, kTestDir), CountryFile(kTestMwm), 1); ScopedDir const scopedDir(kTestDir); string const mwmRelativePath = my::JoinFoldersToPath(kTestDir, kTestMwm + DATA_FILE_EXTENSION); ScopedFile const scopedMwm(mwmRelativePath); @@ -67,9 +67,9 @@ void TestRestrictionBuilding(string const & restrictionContent, string const & m ScopedFile const mappingScopedFile(mappingRelativePath, mappingContent); // Adding restriction section to mwm. - string const restrictionFullPath = my::JoinFoldersToPath(writeableDir, restrictionRelativePath); - string const mappingFullPath = my::JoinFoldersToPath(writeableDir, mappingRelativePath); - string const mwmFullPath = my::JoinFoldersToPath(writeableDir, mwmRelativePath); + string const restrictionFullPath = my::JoinFoldersToPath(writableDir, restrictionRelativePath); + string const mappingFullPath = my::JoinFoldersToPath(writableDir, mappingRelativePath); + string const mwmFullPath = my::JoinFoldersToPath(writableDir, mwmRelativePath); BuildRoadRestrictions(mwmFullPath, restrictionFullPath, mappingFullPath); // Reading from mwm section and testing restrictions. diff --git a/generator/generator_tool/generator_tool.cpp b/generator/generator_tool/generator_tool.cpp index eabe615c49..5c1609b14f 100644 --- a/generator/generator_tool/generator_tool.cpp +++ b/generator/generator_tool/generator_tool.cpp @@ -118,7 +118,7 @@ int main(int argc, char ** argv) genInfo.m_osmFileName = FLAGS_osm_file_name; genInfo.m_restrictions = FLAGS_restriction_name; - genInfo.m_featureId2OsmIds = FLAGS_feature_id_to_osm_ids_name; + genInfo.m_featureIdToOsmIds = FLAGS_feature_id_to_osm_ids_name; genInfo.m_failOnCoasts = FLAGS_fail_on_coasts; genInfo.m_preloadCache = FLAGS_preload_cache; genInfo.m_bookingDatafileName = FLAGS_booking_data; @@ -163,7 +163,7 @@ int main(int argc, char ** argv) LOG(LINFO, ("Generating final data ...")); genInfo.m_restrictions = FLAGS_restriction_name; - genInfo.m_featureId2OsmIds = FLAGS_feature_id_to_osm_ids_name; + genInfo.m_featureIdToOsmIds = FLAGS_feature_id_to_osm_ids_name; genInfo.m_splitByPolygons = FLAGS_split_by_polygons; genInfo.m_createWorld = FLAGS_generate_world; genInfo.m_makeCoasts = FLAGS_make_coasts; @@ -239,11 +239,12 @@ int main(int argc, char ** argv) if (!FLAGS_srtm_path.empty()) routing::BuildRoadAltitudes(datFile, FLAGS_srtm_path); + // @TODO(bykoianko) generate_routing flag should be used for all routing information. if (FLAGS_generate_routing) { routing::BuildRoadRestrictions( - datFile, genInfo.GetIntermediateFileName(genInfo.m_restrictions, ""), - genInfo.GetIntermediateFileName(genInfo.m_featureId2OsmIds, "")); + datFile, genInfo.GetIntermediateFileName(genInfo.m_restrictions, "" /* extention */), + genInfo.GetIntermediateFileName(genInfo.m_featureIdToOsmIds, "" /* extention */)); } } diff --git a/generator/osm_source.cpp b/generator/osm_source.cpp index 5489bf8328..14a318ffe7 100644 --- a/generator/osm_source.cpp +++ b/generator/osm_source.cpp @@ -341,9 +341,8 @@ public: if (info.m_createWorld) m_world.reset(new TWorldGenerator(info)); - // Feature id osm id to map. - string const featureId2OsmIdsFile = info.GetIntermediateFileName(info.m_featureId2OsmIds, ""); - LOG(LINFO, ("Saving osm ids to feature ids map to", featureId2OsmIdsFile)); + string const featureId2OsmIdsFile = info.GetIntermediateFileName(info.m_featureIdToOsmIds, ""); + LOG(LINFO, ("Saving mapping from feature id osm ids to file", featureId2OsmIdsFile)); m_featureId2osmIds.Open(featureId2OsmIdsFile); if (!m_featureId2osmIds.IsOpened()) { @@ -552,9 +551,9 @@ void SyncOfstream::Write(uint32_t featureId, vector const & osmIds) return; lock_guard guard(m_mutex); - m_stream << featureId << ","; + m_stream << featureId; for (osm::Id const & osmId : osmIds) - m_stream << osmId.OsmId() << ","; + m_stream << "," << osmId.OsmId(); m_stream << endl; } diff --git a/generator/restriction_collector.cpp b/generator/restriction_collector.cpp index 3d1a667868..72eda3168f 100644 --- a/generator/restriction_collector.cpp +++ b/generator/restriction_collector.cpp @@ -117,11 +117,11 @@ bool RestrictionCollector::ParseRestrictions(string const & restrictionPath) void RestrictionCollector::ComposeRestrictions() { // Going through all osm id saved in |m_restrictionIndex| (mentioned in restrictions). - size_t const restrictionSz = m_restrictions.size(); - for (pair const & osmIdAndIndex : m_restrictionIndex) + size_t const numRestrictions = m_restrictions.size(); + for (auto const & osmIdAndIndex : m_restrictionIndex) { - Index const & index = osmIdAndIndex.second; - CHECK_LESS(index.m_restrictionNumber, restrictionSz, ()); + LinkIndex const & index = osmIdAndIndex.second; + CHECK_LESS(index.m_restrictionNumber, numRestrictions, ()); Restriction & restriction = m_restrictions[index.m_restrictionNumber]; CHECK_LESS(index.m_linkNumber, restriction.m_links.size(), ()); @@ -157,10 +157,10 @@ void RestrictionCollector::RemoveInvalidRestrictions() void RestrictionCollector::AddRestriction(Restriction::Type type, vector const & osmIds) { + size_t const restrictionCount = m_restrictions.size(); m_restrictions.emplace_back(type, osmIds.size()); - size_t const restrictionCount = m_restrictions.size() - 1; for (size_t i = 0; i < osmIds.size(); ++i) - m_restrictionIndex.emplace_back(osmIds[i], Index({restrictionCount, i})); + m_restrictionIndex.emplace_back(osmIds[i], LinkIndex({restrictionCount, i})); } void RestrictionCollector::AddFeatureId(Restriction::FeatureId featureId, @@ -200,7 +200,8 @@ bool FromString(string str, Restriction::Type & type) } string DebugPrint(Restriction::Type const & type) { return ToString(type); } -string DebugPrint(RestrictionCollector::Index const & index) + +string DebugPrint(RestrictionCollector::LinkIndex const & index) { ostringstream out; out << "m_restrictionNumber:" << index.m_restrictionNumber diff --git a/generator/restriction_collector.hpp b/generator/restriction_collector.hpp index 5e02ef65d2..9b1e984d1d 100644 --- a/generator/restriction_collector.hpp +++ b/generator/restriction_collector.hpp @@ -22,17 +22,19 @@ class RestrictionCollector public: /// \brief Addresses a link in vector. - struct Index + struct LinkIndex { - Index(size_t restrictionNumber, size_t linkNumber) + LinkIndex(size_t restrictionNumber, size_t linkNumber) : m_restrictionNumber(restrictionNumber), m_linkNumber(linkNumber) { } - size_t m_restrictionNumber = 0; // Restriction number in restriction vector. - size_t m_linkNumber = - 0; // Link number for a restriction. It's equal to zero or one for most cases. - bool operator==(Index const & index) const + // Restriction number in restriction vector. + size_t m_restrictionNumber = 0; + // Link number for a restriction. It's equal to zero or one for most cases. + size_t m_linkNumber = 0; + + bool operator==(LinkIndex const & index) const { return m_restrictionNumber == index.m_restrictionNumber && m_linkNumber == index.m_linkNumber; } @@ -56,11 +58,11 @@ private: /// on /// For example: /// 137999, 5170186, - /// 138000, 5170209, + /// 138000, 5170209, 5143342, /// 138001, 5170228, /// \param featureId2OsmIdsPath path to the text file. /// \note Most restrictions consist of type and two linear(road) features. - /// \note For the time being only line-point-line restritions are supported. + /// \note For the time being only line-point-line restrictions are supported. bool ParseFeatureId2OsmIdsMapping(string const & featureId2OsmIdsPath); /// \brief Parses comma separated text file with line in following format: @@ -91,7 +93,7 @@ private: void AddRestriction(Restriction::Type type, vector const & osmIds); RestrictionVec m_restrictions; - vector> m_restrictionIndex; + vector> m_restrictionIndex; unordered_multimap m_osmIds2FeatureId; }; @@ -99,6 +101,6 @@ private: string ToString(Restriction::Type const & type); bool FromString(string str, Restriction::Type & type); string DebugPrint(Restriction::Type const & type); -string DebugPrint(RestrictionCollector::Index const & index); +string DebugPrint(RestrictionCollector::LinkIndex const & index); string DebugPrint(Restriction const & restriction); } // namespace routing diff --git a/generator/restriction_dumper.cpp b/generator/restriction_dumper.cpp index 2cf878fc41..dd10593a90 100644 --- a/generator/restriction_dumper.cpp +++ b/generator/restriction_dumper.cpp @@ -78,14 +78,14 @@ void RestrictionDumper::Write(RelationElement const & relationElement) auto const fromIt = findTag(relationElement.ways, "from"); if (fromIt == relationElement.ways.cend()) - return; // No tag |from| in |relationElement.ways|. + return; auto const toIt = findTag(relationElement.ways, "to"); if (toIt == relationElement.ways.cend()) - return; // No tag |to| in |relationElement.ways|. + return; if (findTag(relationElement.nodes, "via") == relationElement.nodes.cend()) - return; // No tag |via| in |relationElement.nodes|. + return; // Extracting type of restriction. auto const tagIt = relationElement.tags.find("restriction"); diff --git a/generator/restriction_dumper.hpp b/generator/restriction_dumper.hpp index b86a6ababe..e7735d9b1a 100644 --- a/generator/restriction_dumper.hpp +++ b/generator/restriction_dumper.hpp @@ -16,7 +16,7 @@ public: bool IsOpened(); /// \brief Writes |relationElement| to |m_stream| if |relationElement| is a supported restriction. - /// \note For the time being only line-point-line restritions are processed. The other + /// \note For the time being only line-point-line restrictions are processed. The other /// restrictions /// are ignored. // @TODO(bykoianko) It's necessary to process all kind of restrictions. diff --git a/indexer/routing.cpp b/indexer/routing.cpp index 0ff5d646a7..17b53c8729 100644 --- a/indexer/routing.cpp +++ b/indexer/routing.cpp @@ -35,8 +35,8 @@ bool Restriction::operator<(Restriction const & restriction) const namespace feature { -// For the time being only one kind of restrictions is support. It's line-point-line -// restrictions in osm ids term. Such restrictions reflects to two feature ids +// For the time being only one kind of restrictions is supported. It's line-point-line +// restrictions in osm ids term. Such restrictions correspond to two feature ids // restrictions in feature id terms. Because of it supported number of links is two. size_t const RestrictionSerializer::kSupportedLinkNumber = 2; }