[base] Osm::Id fixes.

The masks in the higher bits of our osm id encoding
used to be disjoint. After a refactoring, the encoding
was switched to packing 4 values into 2 bits and thus
the masks started to intersect. The checks were left
untouched, though, resulting in Relation passing tests
for being a Node and a Way.
This commit fixes the bug and introduces an enum to
store the osm id type, hoping to make things more clear.

3 files changed, 55 insertions, 56 deletions

diff --git a/base/base_tests/osm_id_test.cpp b/base/base_tests/osm_id_test.cpp
index d3cd7e7285..e5ecaea065 100644
--- a/base/base_tests/osm_id_test.cpp
+++ b/base/base_tests/osm_id_test.cpp
@@ -2,29 +2,23 @@
 
 #include "base/osm_id.hpp"
 
-using namespace osm;
-
+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(node.GetOsmId(), 12345ULL, ());
+  TEST_EQUAL(node.GetType(), Id::Type::Node, ());
   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(way.GetOsmId(), 93245123456332ULL, ());
+  TEST_EQUAL(way.GetType(), Id::Type::Way, ());
   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(relation.GetOsmId(), 5ULL, ());
+  TEST_EQUAL(relation.GetType(), Id::Type::Relation, ());
   TEST_EQUAL(DebugPrint(relation), "relation 5", ());
 }
+}  // namespace osm
diff --git a/base/osm_id.cpp b/base/osm_id.cpp
index 70c588911f..4fe42ec891 100644
--- a/base/osm_id.cpp
+++ b/base/osm_id.cpp
@@ -1,17 +1,17 @@
 #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.
+// Note that the masks are not disjoint.
 uint64_t const kNode = 0x4000000000000000ULL;
 uint64_t const kWay = 0x8000000000000000ULL;
 uint64_t const kRelation = 0xC000000000000000ULL;
-uint64_t const kReset = ~(kNode | kWay | kRelation);
+uint64_t const kTypeMask = 0xC000000000000000ULL;
 }  // namespace
 
 namespace osm
@@ -22,60 +22,60 @@ Id::Id(uint64_t encodedId) : m_encodedId(encodedId)
 
 Id Id::Node(uint64_t id)
 {
+  ASSERT_EQUAL(id & kTypeMask, 0, ());
   return Id(id | kNode);
 }
 
 Id Id::Way(uint64_t id)
 {
+  ASSERT_EQUAL(id & kTypeMask, 0, ());
   return Id(id | kWay);
 }
 
 Id Id::Relation(uint64_t id)
 {
+  ASSERT_EQUAL(id & kTypeMask, 0, ());
   return Id(id | kRelation);
 }
 
-uint64_t Id::OsmId() const
+uint64_t Id::GetOsmId() const
 {
-  return m_encodedId & kReset;
+  ASSERT_NOT_EQUAL(m_encodedId & kTypeMask, 0, ());
+  return m_encodedId & ~kTypeMask;
 }
 
-uint64_t Id::EncodedId() const
+uint64_t Id::GetEncodedId() const
 {
   return m_encodedId;
 }
 
-bool Id::IsNode() const
+Id::Type Id::GetType() const
 {
-  return (m_encodedId & kNode) == kNode;
+  uint64_t const mask = m_encodedId & kTypeMask;
+  switch (mask)
+  {
+  case kNode: return Id::Type::Node;
+  case kWay: return Id::Type::Way;
+  case kRelation: return Id::Type::Relation;
+  }
+  CHECK_SWITCH();
 }
 
-bool Id::IsWay() const
+std::string DebugPrint(Id::Type const & t)
 {
-  return (m_encodedId & kWay) == kWay;
+  switch (t)
+  {
+  case Id::Type::Node: return "node";
+  case Id::Type::Way: return "way";
+  case Id::Type::Relation: return "relation";
+  }
+  CHECK_SWITCH();
 }
 
-bool Id::IsRelation() const
+std::string DebugPrint(Id const & id)
 {
-  return (m_encodedId & kRelation) == kRelation;
-}
-
-std::string DebugPrint(osm::Id const & id)
-{
-  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
-    typeStr = "ERROR: Not initialized Osm ID";
-
-  std::ostringstream stream;
-  stream << typeStr << " " << id.OsmId();
-  return stream.str();
+  std::ostringstream oss;
+  oss << DebugPrint(id.GetType()) << " " << id.GetOsmId();
+  return oss.str();
 }
 }  // namespace osm
diff --git a/base/osm_id.hpp b/base/osm_id.hpp
index c38f471ef0..515a28dd01 100644
--- a/base/osm_id.hpp
+++ b/base/osm_id.hpp
@@ -9,6 +9,13 @@ namespace osm
 class Id
 {
 public:
+  enum class Type
+  {
+    Node,
+    Way,
+    Relation
+  };
+
   static const uint64_t kInvalid = 0ULL;
 
   explicit Id(uint64_t encodedId = kInvalid);
@@ -17,17 +24,14 @@ public:
   static Id Way(uint64_t osmId);
   static Id Relation(uint64_t osmId);
 
-  uint64_t OsmId() const;
-  uint64_t EncodedId() const;
-
-  bool IsNode() const;
-  bool IsWay() const;
-  bool IsRelation() const;
+  uint64_t GetOsmId() const;
+  uint64_t GetEncodedId() const;
+  Type GetType() const;
 
   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; }
+  bool operator==(uint64_t other) const { return GetOsmId() == other; }
 
 private:
   uint64_t m_encodedId;
@@ -35,8 +39,9 @@ private:
 
 struct HashId : private std::hash<uint64_t>
 {
-  size_t operator()(Id const & id) const { return std::hash<uint64_t>::operator()(id.OsmId()); }
+  size_t operator()(Id const & id) const { return std::hash<uint64_t>::operator()(id.GetOsmId()); }
 };
 
-std::string DebugPrint(osm::Id const & id);
+std::string DebugPrint(Id::Type const & t);
+std::string DebugPrint(Id const & id);
 }  // namespace osm