Review fixes

base/geo_object_id.hpp

@@ -73,8 +73,6 @@ public:
   // Returns the source type of the object.
   Type GetType() const;
 
-  bool IsValid() const { return m_encodedId != kInvalid; }
-
   bool operator<(GeoObjectId const & other) const { return m_encodedId < other.m_encodedId; }
   bool operator==(GeoObjectId const & other) const { return m_encodedId == other.m_encodedId; }
   bool operator!=(GeoObjectId const & other) const { return !(*this == other); }

generator/CMakeLists.txt

@@ -89,8 +89,6 @@ set(SRC
   relation_tags.hpp
   region_meta.cpp
   region_meta.hpp
-  regions.cpp
-  regions.hpp
   regions/city.hpp
   regions/node.cpp
   regions/node.hpp
@@ -100,6 +98,8 @@ set(SRC
   regions/region_base.hpp
   regions/region_info_collector.cpp
   regions/region_info_collector.hpp
+  regions/regions.cpp
+  regions/regions.hpp
   regions/regions_builder.cpp
   regions/regions_builder.hpp
   regions/to_string_policy.cpp

generator/generator_tests/regions_tests.cpp

@@ -1,8 +1,8 @@
 #include "testing/testing.hpp"
 
 #include "generator/osm_element.hpp"
-#include "generator/regions.hpp"
 #include "generator/regions/region_info_collector.hpp"
+#include "generator/regions/regions.hpp"
 #include "generator/regions/regions_builder.hpp"
 #include "generator/regions/to_string_policy.hpp"
 

generator/generator_tool/generator_tool.cpp

@@ -15,7 +15,7 @@
 #include "generator/metalines_builder.hpp"
 #include "generator/osm_source.hpp"
 #include "generator/popular_places_section_builder.hpp"
-#include "generator/regions.hpp"
+#include "generator/regions/regions.hpp"
 #include "generator/restriction_generator.hpp"
 #include "generator/road_access_generator.hpp"
 #include "generator/routing_index_generator.hpp"
@@ -397,7 +397,7 @@ int main(int argc, char ** argv)
     {
       CHECK(FLAGS_generate_region_features, ("Option --generate_regions_kv can be used only "
                                              "together with option --generate_region_features."));
-      if (!GenerateRegions(genInfo))
+      if (!regions::GenerateRegions(genInfo))
       {
         LOG(LCRITICAL, ("Error generating regions kv."));
         return EXIT_FAILURE;

generator/regions.hpp

@@ -1,27 +0,0 @@
-#pragma once
-
-#include "generator/feature_builder.hpp"
-#include "generator/regions/region_info_collector.hpp"
-
-#include "geometry/rect2d.hpp"
-
-
-
-#include "base/geo_object_id.hpp"
-
-#include <cstdint>
-#include <memory>
-#include <string>
-#include <vector>
-
-#include "3party/boost/boost/geometry.hpp"
-
-namespace feature
-{
-struct GenerateInfo;
-}  // namespace feature
-
-namespace generator
-{
-bool GenerateRegions(feature::GenerateInfo const & genInfo); 
-}  // namespace generator

generator/regions/node.cpp

@@ -94,7 +94,7 @@ size_t MaxDepth(Node::Ptr node)
 void PrintTree(Node::Ptr node, std::ostream & stream = std::cout, std::string prefix = "",
                bool isTail = true)
 {
-  auto const & childern = node->GetChildren();
+  auto const & children = node->GetChildren();
   stream << prefix;
   if (isTail)
   {
@@ -115,8 +115,8 @@ void PrintTree(Node::Ptr node, std::ostream & stream = std::cout, std::string pr
          << ";" << static_cast<size_t>(d.GetRank())
          << ";[" << point.get<0>() << "," << point.get<1>() << "])"
          << std::endl;
-  for (size_t i = 0, size = childern.size(); i < size; ++i)
-    PrintTree(childern[i], stream, prefix, i == size - 1);
+  for (size_t i = 0, size = children.size(); i < size; ++i)
+    PrintTree(children[i], stream, prefix, i == size - 1);
 }
 
 void DebugPrintTree(Node::Ptr tree, std::ostream & stream)

generator/regions/node.hpp

@@ -6,8 +6,6 @@
 #include <memory>
 #include <vector>
 
-#include "3party/boost/boost/geometry.hpp"
-
 namespace generator
 {
 namespace regions
@@ -37,13 +35,16 @@ private:
 };
 
 size_t TreeSize(Node::Ptr node);
+
 size_t MaxDepth(Node::Ptr node);
+
 void DebugPrintTree(Node::Ptr tree, std::ostream & stream = std::cout);
+
 // This function merges two trees if the roots have the same ids.
 Node::Ptr MergeTree(Node::Ptr l, Node::Ptr r);
+
 // This function corrects the tree. It traverses the whole node and unites children with
 // the same ids.
 void NormalizeTree(Node::Ptr tree);
-
 }  // namespace regions
 }  // namespace generator

generator/regions/region.cpp

@@ -8,7 +8,7 @@
 #include <algorithm>
 #include <numeric>
 
-#include "3party/boost/boost/geometry.hpp"
+#include <boost/geometry.hpp>
 
 namespace generator
 {
@@ -81,7 +81,7 @@ double Region::CalculateOverlapPercentage(Region const & other) const
   boost::geometry::intersection(*other.m_polygon, *m_polygon, coll);
   auto const min = std::min(boost::geometry::area(*other.m_polygon),
                             boost::geometry::area(*m_polygon));
-  auto const binOp = [] (double x, BoostPolygon const & y) { return x + boost::geometry::area(y); };
+  auto const binOp = [](double x, BoostPolygon const & y) { return x + boost::geometry::area(y); };
   auto const sum = std::accumulate(std::begin(coll), std::end(coll), 0., binOp);
   return (sum / min) * 100;
 }

generator/regions/region_base.hpp

@@ -12,7 +12,7 @@
 #include <cstdint>
 #include <string>
 
-#include "3party/boost/boost/geometry.hpp"
+#include <boost/geometry.hpp>
 
 namespace generator
 {
@@ -51,6 +51,7 @@ struct RegionWithData
   base::GeoObjectId GetAdminCenterId() const;
   bool HasIsoCode() const;
   std::string GetIsoCode() const;
+
   // Absolute rank values do not mean anything. But if the rank of the first object is more than the
   // rank of the second object, then the first object is considered more nested.
   uint8_t GetRank() const;

generator/regions/region_info_collector.cpp

@@ -116,7 +116,7 @@ void RegionInfoCollector::Save(std::string const & filename)
   }
 }
 
-RegionDataProxy RegionInfoCollector::Get(base::GeoObjectId const & osmId) const
+RegionDataProxy RegionInfoCollector::Get(base::GeoObjectId const & osmId)
 {
   return RegionDataProxy(*this, osmId);
 }
@@ -161,7 +161,7 @@ void RegionInfoCollector::FillIsoCode(base::GeoObjectId const & osmId, OsmElemen
   rd.SetNumeric(el.GetTag("ISO3166-1:numeric"));
 }
 
-RegionDataProxy::RegionDataProxy(RegionInfoCollector const & regionInfoCollector,
+RegionDataProxy::RegionDataProxy(RegionInfoCollector & regionInfoCollector,
                                  base::GeoObjectId const & osmId)
   : m_regionInfoCollector(regionInfoCollector),
     m_osmId(osmId)
@@ -173,6 +173,16 @@ RegionInfoCollector const & RegionDataProxy::GetCollector() const
   return m_regionInfoCollector;
 }
 
+RegionInfoCollector & RegionDataProxy::GetCollector()
+{
+  return m_regionInfoCollector;
+}
+
+RegionInfoCollector::MapRegionData & RegionDataProxy::GetMapRegionData()
+{
+  return GetCollector().m_mapRegionData;
+}
+
 
 RegionInfoCollector::MapRegionData const & RegionDataProxy::GetMapRegionData() const
 {
@@ -202,12 +212,12 @@ PlaceType RegionDataProxy::GetPlaceType() const
 
 void RegionDataProxy::SetAdminLevel(AdminLevel adminLevel)
 {
-  const_cast<AdminLevel &>(GetMapRegionData().at(m_osmId).m_adminLevel) = adminLevel;
+  GetMapRegionData().at(m_osmId).m_adminLevel = adminLevel;
 }
 
 void RegionDataProxy::SetPlaceType(PlaceType placeType)
 {
-  const_cast<PlaceType &>(GetMapRegionData().at(m_osmId).m_place) = placeType;
+  GetMapRegionData().at(m_osmId).m_place = placeType;
 }
 
 bool RegionDataProxy::HasAdminLevel() const
@@ -260,12 +270,12 @@ std::string RegionDataProxy::GetIsoCodeAlphaNumeric() const
 bool RegionDataProxy::HasAdminCenter() const
 {
   return (GetMapRegionData().count(m_osmId) != 0) &&
-      (GetMapRegionData().at(m_osmId).m_osmIdAdminCenter.IsValid());
+      (GetMapRegionData().at(m_osmId).m_osmIdAdminCenter.HasId());
 }
 
 base::GeoObjectId RegionDataProxy::GetAdminCenter() const
 {
-  return GetMapRegionData().at(m_osmId).m_osmIdAdminCenter;
+  return GetMapRegionData().at(m_osmId).m_osmIdAdminCenter.GetId();
 }
 }  // namespace regions
 }  // namespace generator

generator/regions/region_info_collector.hpp

@@ -69,7 +69,7 @@ public:
   explicit RegionInfoCollector(std::string const & filename);
   explicit RegionInfoCollector(Platform::FilesList const & filenames);
   void Add(base::GeoObjectId const & osmId, OsmElement const & el);
-  RegionDataProxy Get(base::GeoObjectId const & osmId) const;
+  RegionDataProxy Get(base::GeoObjectId const & osmId);
   void Save(std::string const & filename);
 
 private:
@@ -98,12 +98,25 @@ private:
     char m_numeric[4] = {};
   };
 
+  struct AdminCenter
+  {
+    AdminCenter() : m_has(false) {}
+    AdminCenter(base::GeoObjectId const & id) : m_has(true), m_id(id) {}
+
+    bool HasId() const { return m_has; }
+    base::GeoObjectId GetId() const { return m_id; }
+
+  private:
+    bool m_has;
+    base::GeoObjectId m_id;
+  };
+
   struct RegionData
   {
     base::GeoObjectId m_osmId;
     AdminLevel m_adminLevel = AdminLevel::Unknown;
     PlaceType m_place = PlaceType::Unknown;
-    base::GeoObjectId m_osmIdAdminCenter;
+    AdminCenter m_osmIdAdminCenter;
   };
 
   using MapRegionData = std::unordered_map<base::GeoObjectId, RegionData>;
@@ -144,7 +157,7 @@ private:
 class RegionDataProxy
 {
 public:
-  RegionDataProxy(RegionInfoCollector const & regionInfoCollector, base::GeoObjectId const & osmId);
+  RegionDataProxy(RegionInfoCollector & regionInfoCollector, base::GeoObjectId const & osmId);
 
   base::GeoObjectId const & GetOsmId() const;
   AdminLevel GetAdminLevel() const;
@@ -169,11 +182,13 @@ public:
 
 private:
   bool HasIsoCode() const;
+  RegionInfoCollector & GetCollector();
   RegionInfoCollector const & GetCollector() const;
+  RegionInfoCollector::MapRegionData & GetMapRegionData();
   RegionInfoCollector::MapRegionData const & GetMapRegionData() const;
   RegionInfoCollector::MapIsoCode const & GetMapIsoCode() const;
 
-  std::reference_wrapper<RegionInfoCollector const> m_regionInfoCollector;
+  std::reference_wrapper<RegionInfoCollector> m_regionInfoCollector;
   base::GeoObjectId m_osmId;
 };
 

generator/regions/regions.cpp

@@ -1,4 +1,4 @@
-#include "generator/regions.hpp"
+#include "generator/regions/regions.hpp"
 
 #include "generator/feature_builder.hpp"
 #include "generator/generate_info.hpp"
@@ -13,6 +13,7 @@
 #include "coding/transliteration.hpp"
 
 #include "base/logging.hpp"
+#include "base/stl_helpers.hpp"
 #include "base/timer.hpp"
 
 #include <algorithm>
@@ -24,6 +25,7 @@
 #include <string>
 #include <thread>
 #include <tuple>
+#include <unordered_map>
 #include <unordered_set>
 #include <vector>
 
@@ -68,14 +70,14 @@ struct RegionsFixer
       auto const placeType = adminCenter.GetPlaceType();
       if (placeType == PlaceType::Town || placeType == PlaceType::City)
       {
-        for (size_t j =  i + 1; j < m_regionsWithAdminCenter.size() - 1; ++j)
+        for (size_t j = i + 1; j + 1 < m_regionsWithAdminCenter.size(); ++j)
         {
           if (m_regionsWithAdminCenter[j].ContainsRect(regionWithAdminCenter))
             unsuitable[j] = true;
         }
       }
 
-      if (ExistsRegionAsCity(adminCenter))
+      if (RegionExistsAsCity(adminCenter))
         continue;
 
       regionWithAdminCenter.SetInfo(adminCenter);
@@ -88,7 +90,7 @@ struct RegionsFixer
   }
 
 private:
-  bool ExistsRegionAsCity(const City & city)
+  bool RegionExistsAsCity(City const & city)
   {
     auto const range = m_nameRegionMap.equal_range(city.GetName());
     for (auto it = range.first; it != range.second; ++it)
@@ -103,11 +105,10 @@ private:
 
   void SplitRegionsByAdminCenter()
   {
-    auto const pred = [](Region const & region) { return region.GetAdminCenterId().IsValid(); };
+    auto const pred = [](Region const & region) { return region.HasAdminCenter(); };
     std::copy_if(std::begin(m_regions), std::end(m_regions),
                  std::back_inserter(m_regionsWithAdminCenter), pred);
-    auto const it =  std::remove_if(std::begin(m_regions), std::end(m_regions), pred);
-    m_regions.erase(it, std::end(m_regions));
+    base::EraseIf(m_regions, pred);
   }
 
   void CreateNameRegionMap()
@@ -133,7 +134,7 @@ private:
 };
 
 std::tuple<RegionsBuilder::Regions, PointCitiesMap>
-ReadDatasetFromTmpMwm(feature::GenerateInfo const & genInfo, RegionInfoCollector const & collector)
+ReadDatasetFromTmpMwm(feature::GenerateInfo const & genInfo, RegionInfoCollector & collector)
 {
   RegionsBuilder::Regions regions;
   PointCitiesMap pointCitiesMap;
@@ -154,7 +155,7 @@ ReadDatasetFromTmpMwm(feature::GenerateInfo const & genInfo, RegionInfoCollector
   };
 
   feature::ForEachFromDatRawFormat(tmpMwmFilename, toDo);
-  return std::make_tuple(regions, pointCitiesMap);
+  return std::make_tuple(std::move(regions), std::move(pointCitiesMap));
 }
 
 void FilterRegions(RegionsBuilder::Regions & regions)
@@ -170,7 +171,7 @@ void FilterRegions(RegionsBuilder::Regions & regions)
 }
 
 RegionsBuilder::Regions ReadData(feature::GenerateInfo const & genInfo,
-                                 RegionInfoCollector const & regionsInfoCollector)
+                                 RegionInfoCollector & regionsInfoCollector)
 {
   RegionsBuilder::Regions regions;
   PointCitiesMap pointCitiesMap;
@@ -180,9 +181,7 @@ RegionsBuilder::Regions ReadData(feature::GenerateInfo const & genInfo,
   FilterRegions(regions);
   return std::move(regions);
 }
-
 }  // namespace
-}  // namespace regions
 
 bool GenerateRegions(feature::GenerateInfo const & genInfo)
 {
@@ -207,6 +206,9 @@ bool GenerateRegions(feature::GenerateInfo const & genInfo)
   for (auto const & countryName : kvBuilder->GetCountryNames())
   {
     auto const tree = kvBuilder->GetNormalizedCountryTree(countryName);
+    if (!tree)
+      continue;
+
     if (genInfo.m_verbose)
       DebugPrintTree(tree);
 
@@ -224,4 +226,5 @@ bool GenerateRegions(feature::GenerateInfo const & genInfo)
   LOG(LINFO, ("Finish generating regions.", timer.ElapsedSeconds(), "seconds."));
   return true;
 }
+}  // namespace regions
 }  // namespace generator

generator/regions/regions.hpp

@@ -0,0 +1,14 @@
+#pragma once
+
+namespace feature
+{
+struct GenerateInfo;
+}  // namespace feature
+
+namespace generator
+{
+namespace regions
+{
+bool GenerateRegions(feature::GenerateInfo const & genInfo); 
+}  // namespace regions
+}  // namespace generator

generator/regions/regions_builder.cpp

@@ -1,18 +1,16 @@
 #include "generator/regions/regions_builder.hpp"
 
 #include "base/assert.hpp"
+#include "base/stl_helpers.hpp"
 
 #include <algorithm>
 #include <chrono>
 #include <fstream>
 #include <functional>
 #include <numeric>
-#include <memory>
 #include <queue>
-#include <string>
 #include <thread>
 #include <unordered_set>
-#include <vector>
 
 #include "3party/ThreadPool/ThreadPool.h"
 
@@ -29,10 +27,9 @@ RegionsBuilder::RegionsBuilder(Regions && regions,
   ASSERT(m_toStringPolicy, ());
   ASSERT(m_cpuCount != 0, ());
 
-  auto const isCountry = [](Region const & r){ return r.IsCountry(); };
+  auto const isCountry = [](Region const & r) { return r.IsCountry(); };
   std::copy_if(std::begin(regions), std::end(regions), std::back_inserter(m_countries), isCountry);
-  auto const it = std::remove_if(std::begin(regions), std::end(regions), isCountry);
-  regions.erase(it, std::end(regions));
+  base::EraseIf(regions, isCountry);
   auto const cmp = [](Region const & l, Region const & r) { return l.GetArea() > r.GetArea(); };
   std::sort(std::begin(m_countries), std::end(m_countries), cmp);
 
@@ -173,14 +170,14 @@ void RegionsBuilder::MakeCountryTrees(Regions const & regions)
     ThreadPool threadPool(cpuCount);
     for (auto const & country : GetCountries())
     {
-      auto f = threadPool.enqueue(&RegionsBuilder::BuildCountryRegionTree, country, regions);
-      results.emplace_back(std::move(f));
+      auto result = threadPool.enqueue(&RegionsBuilder::BuildCountryRegionTree, country, regions);
+      results.emplace_back(std::move(result));
     }
   }
 
-  for (auto & r : results)
+  for (auto & result : results)
   {
-    auto tree = r.get();
+    auto tree = result.get();
     m_countryTrees.emplace(tree->GetData().GetName(), std::move(tree));
   }
 }

generator/regions/regions_builder.hpp

@@ -4,7 +4,10 @@
 #include "generator/regions/region.hpp"
 #include "generator/regions/to_string_policy.hpp"
 
+#include <map>
 #include <memory>
+#include <string>
+#include <vector>
 
 namespace generator
 {
@@ -42,6 +45,5 @@ private:
   Regions m_countries;
   int m_cpuCount;
 };
-
 }  // namespace regions
 }  // namespace generator

generator/regions/to_string_policy.cpp

@@ -2,8 +2,9 @@
 
 #include <memory>
 
+#include <boost/range/adaptor/reversed.hpp>
+
 #include "3party/jansson/myjansson.hpp"
-#include "3party/boost/boost/range/adaptor/reversed.hpp"
 
 namespace generator
 {

generator/regions/to_string_policy.hpp

@@ -16,7 +16,6 @@ public:
   virtual std::string ToString(Node::PtrList const & nodePtrList) = 0;
 };
 
-
 class JsonPolicy : public ToStringPolicyInterface
 {
 public:

generator/translator_region.cpp

@@ -66,7 +66,7 @@ bool TranslatorRegion::IsSuitableElement(OsmElement const * p) const
 
     auto const & members = p->Members();
     auto const pred = [](OsmElement::Member const & m) { return m.role == "admin_centre"; };
-    if (t.key == "boundary" && t.value == "police" &&
+    if (t.key == "boundary" && t.value == "political" &&
         std::find_if(std::begin(members), std::end(members), pred) != std::end(members))
       return true;