Review fixes.

7 files changed, 59 insertions, 45 deletions

diff --git a/coding/coding_tests/geometry_coding_test.cpp b/coding/coding_tests/geometry_coding_test.cpp
index 3e398a17b3..0af921ffec 100644
--- a/coding/coding_tests/geometry_coding_test.cpp
+++ b/coding/coding_tests/geometry_coding_test.cpp
@@ -20,7 +20,7 @@ using PU = m2::PointU;
 
 namespace
 {
-m2::PointU D2U(m2::PointD const & p) { return PointDToPointU(p, POINT_COORD_BITS); }
+m2::PointU D2U(m2::PointD const & p) { return PointDToPointU(p, kPointCoordBits); }
 
 m2::PointU GetMaxPoint() { return D2U(m2::PointD(MercatorBounds::kMaxX, MercatorBounds::kMaxY)); }
 
diff --git a/coding/coding_tests/geometry_serialization_test.cpp b/coding/coding_tests/geometry_serialization_test.cpp
index 6e858ab14a..25937556b7 100644
--- a/coding/coding_tests/geometry_serialization_test.cpp
+++ b/coding/coding_tests/geometry_serialization_test.cpp
@@ -8,26 +8,31 @@
 #include "geometry/mercator.hpp"
 
 #include "base/logging.hpp"
+#include "base/math.hpp"
 
-// Copy-Paste from feature_builder.cpp
+#include <vector>
+
+using namespace std;
+
+// Copy-Paste from generator/feature_builder.cpp
 namespace
 {
-bool is_equal(double d1, double d2)
+bool IsEqual(double d1, double d2)
 {
-  return (fabs(d1 - d2) < kCellIdToPointEps);
+  return base::AlmostEqualAbs(d1, d2, kCellIdToPointEps);
 }
 
-bool is_equal(m2::PointD const & p1, m2::PointD const & p2)
+bool IsEqual(m2::PointD const & p1, m2::PointD const & p2)
 {
   return p1.EqualDxDy(p2, kCellIdToPointEps);
 }
 
-bool is_equal(m2::RectD const & r1, m2::RectD const & r2)
+bool IsEqual(m2::RectD const & r1, m2::RectD const & r2)
 {
-  return (is_equal(r1.minX(), r2.minX()) && is_equal(r1.minY(), r2.minY()) &&
-          is_equal(r1.maxX(), r2.maxX()) && is_equal(r1.maxY(), r2.maxY()));
-}
+  return (IsEqual(r1.minX(), r2.minX()) && IsEqual(r1.minY(), r2.minY()) &&
+          IsEqual(r1.maxX(), r2.maxX()) && IsEqual(r1.maxY(), r2.maxY()));
 }
+}  // namespace
 
 UNIT_TEST(SaveLoadPolyline_DataSet1)
 {
@@ -53,8 +58,8 @@ UNIT_TEST(SaveLoadPolyline_DataSet1)
     r1.Add(data1[i]);
     r2.Add(data2[i]);
 
-    TEST(is_equal(data1[i], data2[i]), (data1[i], data2[i]));
+    TEST(IsEqual(data1[i], data2[i]), (data1[i], data2[i]));
   }
 
-  TEST(is_equal(r1, r2), (r1, r2));
+  TEST(IsEqual(r1, r2), (r1, r2));
 }
diff --git a/coding/coding_tests/point_coding_tests.cpp b/coding/coding_tests/point_coding_tests.cpp
index d5a0a8dc96..03dc73f63d 100644
--- a/coding/coding_tests/point_coding_tests.cpp
+++ b/coding/coding_tests/point_coding_tests.cpp
@@ -15,7 +15,7 @@ using namespace std;
 namespace
 {
 double const kEps = kCellIdToPointEps;
-uint32_t const kCoordBits = POINT_COORD_BITS;
+uint8_t const kCoordBits = kPointCoordBits;
 uint32_t const kBig = uint32_t{1} << 30;
 
 void CheckEqualPoints(m2::PointD const & p1, m2::PointD const & p2)
diff --git a/coding/geometry_coding.cpp b/coding/geometry_coding.cpp
index 6dd2cd2ccb..1ef1bb8f5c 100644
--- a/coding/geometry_coding.cpp
+++ b/coding/geometry_coding.cpp
@@ -296,7 +296,7 @@ void DecodeTriangleStrip(InDeltasT const & deltas, m2::PointU const & basePoint,
 namespace serial
 {
 // GeometryCodingParams ----------------------------------------------------------------------------
-GeometryCodingParams::GeometryCodingParams() : m_BasePointUint64(0), m_CoordBits(POINT_COORD_BITS)
+GeometryCodingParams::GeometryCodingParams() : m_BasePointUint64(0), m_CoordBits(kPointCoordBits)
 {
   m_BasePoint = Uint64ToPointUObsolete(m_BasePointUint64);
 }
diff --git a/coding/geometry_coding.hpp b/coding/geometry_coding.hpp
index 28f7869f32..bfe2b52205 100644
--- a/coding/geometry_coding.hpp
+++ b/coding/geometry_coding.hpp
@@ -114,7 +114,7 @@ public:
 
   void SetBasePoint(m2::PointD const & pt);
 
-  uint32_t GetCoordBits() const { return m_CoordBits; }
+  uint8_t GetCoordBits() const { return m_CoordBits; }
 
   template <typename WriterT>
   void Save(WriterT & writer) const
diff --git a/coding/point_coding.cpp b/coding/point_coding.cpp
index bb65162750..9670a4eeb2 100644
--- a/coding/point_coding.cpp
+++ b/coding/point_coding.cpp
@@ -8,27 +8,29 @@
 
 namespace
 {
-double CoordSize(uint32_t coordBits) { return (1 << coordBits) - 1; }
+double CoordSize(uint8_t coordBits)
+{
+  ASSERT_LESS_OR_EQUAL(coordBits, 32, ());
+  return static_cast<double>((uint64_t{1} << coordBits) - 1);
+}
 }  // namespace
 
-uint32_t DoubleToUint32(double x, double min, double max, uint32_t coordBits)
+uint32_t DoubleToUint32(double x, double min, double max, uint8_t coordBits)
 {
   ASSERT_GREATER_OR_EQUAL(coordBits, 1, ());
   ASSERT_LESS_OR_EQUAL(coordBits, 32, ());
   x = base::clamp(x, min, max);
-  uint64_t const fullMask = bits::GetFullMask(static_cast<uint8_t>(coordBits));
-  return static_cast<uint32_t>(0.5 + (x - min) / (max - min) * fullMask);
+  return static_cast<uint32_t>(0.5 + (x - min) / (max - min) * bits::GetFullMask(coordBits));
 }
 
-double Uint32ToDouble(uint32_t x, double min, double max, uint32_t coordBits)
+double Uint32ToDouble(uint32_t x, double min, double max, uint8_t coordBits)
 {
   ASSERT_GREATER_OR_EQUAL(coordBits, 1, ());
   ASSERT_LESS_OR_EQUAL(coordBits, 32, ());
-  return min +
-         static_cast<double>(x) * (max - min) / bits::GetFullMask(static_cast<uint8_t>(coordBits));
+  return min + static_cast<double>(x) * (max - min) / bits::GetFullMask(coordBits);
 }
 
-m2::PointU PointDToPointU(double x, double y, uint32_t coordBits)
+m2::PointU PointDToPointU(double x, double y, uint8_t coordBits)
 {
   x = base::clamp(x, MercatorBounds::kMinX, MercatorBounds::kMaxX);
   y = base::clamp(y, MercatorBounds::kMinY, MercatorBounds::kMaxY);
@@ -44,12 +46,12 @@ m2::PointU PointDToPointU(double x, double y, uint32_t coordBits)
   return m2::PointU(ix, iy);
 }
 
-m2::PointU PointDToPointU(m2::PointD const & pt, uint32_t coordBits)
+m2::PointU PointDToPointU(m2::PointD const & pt, uint8_t coordBits)
 {
   return PointDToPointU(pt.x, pt.y, coordBits);
 }
 
-m2::PointD PointUToPointD(m2::PointU const & pt, uint32_t coordBits)
+m2::PointD PointUToPointD(m2::PointU const & pt, uint8_t coordBits)
 {
   return m2::PointD(static_cast<double>(pt.x) * MercatorBounds::kRangeX / CoordSize(coordBits) +
                         MercatorBounds::kMinX,
@@ -59,35 +61,35 @@ m2::PointD PointUToPointD(m2::PointU const & pt, uint32_t coordBits)
 
 // Obsolete functions ------------------------------------------------------------------------------
 
-int64_t PointToInt64Obsolete(double x, double y, uint32_t coordBits)
+int64_t PointToInt64Obsolete(double x, double y, uint8_t coordBits)
 {
   int64_t const res = static_cast<int64_t>(PointUToUint64Obsolete(PointDToPointU(x, y, coordBits)));
 
   ASSERT_GREATER_OR_EQUAL(res, 0, ("Highest bits of (ix, iy) are not used, so res should be > 0."));
-  ASSERT_LESS_OR_EQUAL(static_cast<uint64_t>(res), uint64_t{3} << 2 * POINT_COORD_BITS, ());
+  ASSERT_LESS_OR_EQUAL(static_cast<uint64_t>(res), uint64_t{3} << 2 * kPointCoordBits, ());
   return res;
 }
 
-int64_t PointToInt64Obsolete(m2::PointD const & pt, uint32_t coordBits)
+int64_t PointToInt64Obsolete(m2::PointD const & pt, uint8_t coordBits)
 {
   return PointToInt64Obsolete(pt.x, pt.y, coordBits);
 }
 
-m2::PointD Int64ToPointObsolete(int64_t v, uint32_t coordBits)
+m2::PointD Int64ToPointObsolete(int64_t v, uint8_t coordBits)
 {
   ASSERT_GREATER_OR_EQUAL(v, 0, ("Highest bits of (ix, iy) are not used, so res should be > 0."));
-  ASSERT_LESS_OR_EQUAL(static_cast<uint64_t>(v), uint64_t{3} << 2 * POINT_COORD_BITS, ());
+  ASSERT_LESS_OR_EQUAL(static_cast<uint64_t>(v), uint64_t{3} << 2 * kPointCoordBits, ());
   return PointUToPointD(Uint64ToPointUObsolete(static_cast<uint64_t>(v)), coordBits);
 }
 
-std::pair<int64_t, int64_t> RectToInt64Obsolete(m2::RectD const & r, uint32_t coordBits)
+std::pair<int64_t, int64_t> RectToInt64Obsolete(m2::RectD const & r, uint8_t coordBits)
 {
   int64_t const p1 = PointToInt64Obsolete(r.minX(), r.minY(), coordBits);
   int64_t const p2 = PointToInt64Obsolete(r.maxX(), r.maxY(), coordBits);
   return std::make_pair(p1, p2);
 }
 
-m2::RectD Int64ToRectObsolete(std::pair<int64_t, int64_t> const & p, uint32_t coordBits)
+m2::RectD Int64ToRectObsolete(std::pair<int64_t, int64_t> const & p, uint8_t coordBits)
 {
   m2::PointD const pt1 = Int64ToPointObsolete(p.first, coordBits);
   m2::PointD const pt2 = Int64ToPointObsolete(p.second, coordBits);
diff --git a/coding/point_coding.hpp b/coding/point_coding.hpp
index c8dd7e930d..6d0226569e 100644
--- a/coding/point_coding.hpp
+++ b/coding/point_coding.hpp
@@ -6,41 +6,48 @@
 #include <cstdint>
 #include <utility>
 
-#define POINT_COORD_BITS 30
+uint8_t constexpr kPointCoordBits = 30;
+
+uint8_t constexpr kFeatureSorterPointCoordBits = 27;
 
 // The absolute precision of the point encoding in the mwm files.
 // If both x and y coordinates of two points lie within |kMwmPointAccuracy| of one
 // another we consider the points equal. In other words, |kMwmPointAccuracy| may
 // be used as the eps value for both x and y in Point::EqualDxDy, base::AlmostEqualAbs and such.
 //
-// The constant is loosely tied to MercatorBounds::kRangeX / (1 << POINT_COORD_BITS):
+// The constant is loosely tied to MercatorBounds::kRangeX / (1 << kPointCoordBits):
 //   The range of possible values for point coordinates
 //      MercatorBounds::kRangeX = 360.0
 //   The number of distinct values for each coordinate after encoding
-//      (1 << POINT_COORD_BITS) = 1073741824 ≈ 1e9
+//      (1 << kPointCoordBits) = 1073741824 ≈ 1e9
 //   Distance between two discernible points in the uniform case
 //      360.0 / 1e9 ≈ 4e-7 ≈ 0.04 * |kMwmPointAccuracy|.
 //
 // On the other hand, this should be enough for most purposes because
-// 1e-5 difference in the coordinates of a mercator-proected point corresponds to roughly
+// 1e-5 difference in the coordinates of a mercator-projected point corresponds to roughly
 // 1 meter difference on the equator and we do not expect most OSM points to be mapped
 // with better precision.
 //
 // todo(@m) By this argument, it seems that 1e-6 is a better choice.
+//
+// Note. generator/feature_sorter.cpp uses |kFeatureSorterPointCoordBits|,
+// effectively overriding |kPointCoordBits|. Presumably it does so to guarantee a maximum of
+// 4 bytes in the varint encoding, (27+1 sign(?) bit) / 7 = 4.
+// todo(@m) Clarify how kPointCoordBits and kFeatureSorterPointCoordBits are related.
 double constexpr kMwmPointAccuracy = 1e-5;
 
 // todo(@m) Explain this constant.
 double constexpr kCellIdToPointEps = 1e-4;
 
-uint32_t DoubleToUint32(double x, double min, double max, uint32_t coordBits);
+uint32_t DoubleToUint32(double x, double min, double max, uint8_t coordBits);
 
-double Uint32ToDouble(uint32_t x, double min, double max, uint32_t coordBits);
+double Uint32ToDouble(uint32_t x, double min, double max, uint8_t coordBits);
 
-m2::PointU PointDToPointU(double x, double y, uint32_t coordBits);
+m2::PointU PointDToPointU(double x, double y, uint8_t coordBits);
 
-m2::PointU PointDToPointU(m2::PointD const & pt, uint32_t coordBits);
+m2::PointU PointDToPointU(m2::PointD const & pt, uint8_t coordBits);
 
-m2::PointD PointUToPointD(m2::PointU const & p, uint32_t coordBits);
+m2::PointD PointUToPointD(m2::PointU const & p, uint8_t coordBits);
 
 // All functions below are deprecated and are left
 // only for backward compatibility.
@@ -62,15 +69,15 @@ m2::PointD PointUToPointD(m2::PointU const & p, uint32_t coordBits);
 // when implementing the Z-order curve but we have this
 // written elsewhere (see geometry/cellid.hpp).
 
-int64_t PointToInt64Obsolete(double x, double y, uint32_t coordBits);
+int64_t PointToInt64Obsolete(double x, double y, uint8_t coordBits);
 
-int64_t PointToInt64Obsolete(m2::PointD const & pt, uint32_t coordBits);
+int64_t PointToInt64Obsolete(m2::PointD const & pt, uint8_t coordBits);
 
-m2::PointD Int64ToPointObsolete(int64_t v, uint32_t coordBits);
+m2::PointD Int64ToPointObsolete(int64_t v, uint8_t coordBits);
 
-std::pair<int64_t, int64_t> RectToInt64Obsolete(m2::RectD const & r, uint32_t coordBits);
+std::pair<int64_t, int64_t> RectToInt64Obsolete(m2::RectD const & r, uint8_t coordBits);
 
-m2::RectD Int64ToRectObsolete(std::pair<int64_t, int64_t> const & p, uint32_t coordBits);
+m2::RectD Int64ToRectObsolete(std::pair<int64_t, int64_t> const & p, uint8_t coordBits);
 
 uint64_t PointUToUint64Obsolete(m2::PointU const & pt);