[base] Minor refactoring of osm::Id.

4 files changed, 71 insertions, 39 deletions

diff --git a/base/base_tests/CMakeLists.txt b/base/base_tests/CMakeLists.txt
index 253173f2ee..095e83de9a 100644
--- a/base/base_tests/CMakeLists.txt
+++ b/base/base_tests/CMakeLists.txt
@@ -23,6 +23,7 @@ set(
   move_to_front_tests.cpp
   newtype_test.cpp
   observer_list_test.cpp
+  osm_id_test.cpp
   range_iterator_test.cpp
   ref_counted_tests.cpp
   regexp_test.cpp
diff --git a/base/base_tests/osm_id_test.cpp b/base/base_tests/osm_id_test.cpp
new file mode 100644
index 0000000000..d3cd7e7285
--- /dev/null
+++ b/base/base_tests/osm_id_test.cpp
@@ -0,0 +1,30 @@
+#include "testing/testing.hpp"
+
+#include "base/osm_id.hpp"
+
+using namespace osm;
+
+UNIT_TEST(OsmId)
+{
+  Id const node = Id::Node(12345);
+  TEST_EQUAL(node.OsmId(), 12345ULL, ());
+  TEST(node.IsNode(), ());
+  TEST(!node.IsWay(), ());
+  TEST(!node.IsRelation(), ());
+  TEST_EQUAL(DebugPrint(node), "node 12345", ());
+
+  Id const way = Id::Way(93245123456332ULL);
+  TEST_EQUAL(way.OsmId(), 93245123456332ULL, ());
+  TEST(!way.IsNode(), ());
+  TEST(way.IsWay(), ());
+  TEST(!way.IsRelation(), ());
+  TEST_EQUAL(DebugPrint(way), "way 93245123456332", ());
+
+  Id const relation = Id::Relation(5);
+  TEST_EQUAL(relation.OsmId(), 5ULL, ());
+  // sic!
+  TEST(relation.IsNode(), ());
+  TEST(relation.IsWay(), ());
+  TEST(relation.IsRelation(), ());
+  TEST_EQUAL(DebugPrint(relation), "relation 5", ());
+}
diff --git a/base/osm_id.cpp b/base/osm_id.cpp
index da21db1b45..70c588911f 100644
--- a/base/osm_id.cpp
+++ b/base/osm_id.cpp
@@ -1,41 +1,43 @@
 #include "base/osm_id.hpp"
 
-#include "base/assert.hpp"
-
 #include <sstream>
 
+namespace
+{
+// Use 2 higher bits to encode type.
+//
+// todo The masks are not disjoint for some reason
+//      since the commit 60414dc86254aed22ac9e66fed49eba554260a2c.
+uint64_t const kNode = 0x4000000000000000ULL;
+uint64_t const kWay = 0x8000000000000000ULL;
+uint64_t const kRelation = 0xC000000000000000ULL;
+uint64_t const kReset = ~(kNode | kWay | kRelation);
+}  // namespace
 
 namespace osm
 {
-
-// Use 3 higher bits to encode type
-static const uint64_t NODE = 0x4000000000000000ULL;
-static const uint64_t WAY = 0x8000000000000000ULL;
-static const uint64_t RELATION = 0xC000000000000000ULL;
-static const uint64_t RESET = ~(NODE | WAY | RELATION);
-
 Id::Id(uint64_t encodedId) : m_encodedId(encodedId)
 {
 }
 
 Id Id::Node(uint64_t id)
 {
-  return Id( id | NODE );
+  return Id(id | kNode);
 }
 
 Id Id::Way(uint64_t id)
 {
-  return Id( id | WAY );
+  return Id(id | kWay);
 }
 
 Id Id::Relation(uint64_t id)
 {
-  return Id( id | RELATION );
+  return Id(id | kRelation);
 }
 
 uint64_t Id::OsmId() const
 {
-  return m_encodedId & RESET;
+  return m_encodedId & kReset;
 }
 
 uint64_t Id::EncodedId() const
@@ -45,35 +47,35 @@ uint64_t Id::EncodedId() const
 
 bool Id::IsNode() const
 {
-  return ((m_encodedId & NODE) == NODE);
+  return (m_encodedId & kNode) == kNode;
 }
 
 bool Id::IsWay() const
 {
-  return ((m_encodedId & WAY) == WAY);
+  return (m_encodedId & kWay) == kWay;
 }
 
 bool Id::IsRelation() const
 {
-  return ((m_encodedId & RELATION) == RELATION);
+  return (m_encodedId & kRelation) == kRelation;
 }
 
-std::string Id::Type() const
+std::string DebugPrint(osm::Id const & id)
 {
-  if ((m_encodedId & RELATION) == RELATION)
-    return "relation";
-  else if ((m_encodedId & NODE) == NODE)
-    return "node";
-  else if ((m_encodedId & WAY) == WAY)
-    return "way";
+  std::string typeStr;
+  // Note that with current encoding all relations are also at the
+  // same time nodes and ways. Therefore, the relation check must go first.
+  if (id.IsRelation())
+    typeStr = "relation";
+  else if (id.IsNode())
+    typeStr = "node";
+  else if (id.IsWay())
+    typeStr = "way";
   else
-    return "ERROR: Not initialized Osm ID";
-}
+    typeStr = "ERROR: Not initialized Osm ID";
 
-std::string DebugPrint(osm::Id const & id)
-{
   std::ostringstream stream;
-  stream << id.Type() << " " << id.OsmId();
+  stream << typeStr << " " << id.OsmId();
   return stream.str();
 }
 }  // namespace osm
diff --git a/base/osm_id.hpp b/base/osm_id.hpp
index 597f81eb05..c38f471ef0 100644
--- a/base/osm_id.hpp
+++ b/base/osm_id.hpp
@@ -8,12 +8,10 @@ namespace osm
 {
 class Id
 {
-  uint64_t m_encodedId;
-
-  static const uint64_t INVALID = 0ULL;
-
 public:
-  explicit Id(uint64_t encodedId = INVALID);
+  static const uint64_t kInvalid = 0ULL;
+
+  explicit Id(uint64_t encodedId = kInvalid);
 
   static Id Node(uint64_t osmId);
   static Id Way(uint64_t osmId);
@@ -21,17 +19,18 @@ public:
 
   uint64_t OsmId() const;
   uint64_t EncodedId() const;
+
   bool IsNode() const;
   bool IsWay() const;
   bool IsRelation() const;
 
-  /// For debug output
-  std::string Type() const;
-
-  inline bool operator<(Id const & other) const { return m_encodedId < other.m_encodedId; }
-  inline bool operator==(Id const & other) const { return m_encodedId == other.m_encodedId; }
-  inline bool operator!=(Id const & other) const { return !(*this == other); }
+  bool operator<(Id const & other) const { return m_encodedId < other.m_encodedId; }
+  bool operator==(Id const & other) const { return m_encodedId == other.m_encodedId; }
+  bool operator!=(Id const & other) const { return !(*this == other); }
   bool operator==(uint64_t other) const { return OsmId() == other; }
+
+private:
+  uint64_t m_encodedId;
 };
 
 struct HashId : private std::hash<uint64_t>