From e1fd6bacb5b9638d4e2efe9c3db034191a0ec408 Mon Sep 17 00:00:00 2001
From: Nathan Hughes <na26933@mit.edu>
Date: Sat, 14 Sep 2024 14:41:09 +0000
Subject: [PATCH] remove implicit bool conversion

---
 include/spark_dsg/layer_prefix.h |  5 ++---
 src/dynamic_scene_graph.cpp      | 12 +++++++++---
 src/layer_prefix.cpp             |  8 +++++---
 src/scene_graph_utilities.cpp    |  2 +-
 tests/utest_layer_prefix.cpp     |  7 -------
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/spark_dsg/layer_prefix.h b/include/spark_dsg/layer_prefix.h
index aacf14b..67de56c 100644
--- a/include/spark_dsg/layer_prefix.h
+++ b/include/spark_dsg/layer_prefix.h
@@ -43,7 +43,6 @@
 namespace spark_dsg {
 
 struct LayerKey {
-  static constexpr LayerId UNKNOWN_LAYER = DsgLayers::UNKNOWN;
   LayerId layer;
   uint32_t prefix = 0;
   bool dynamic = false;
@@ -56,13 +55,13 @@ struct LayerKey {
 
   bool isParent(const LayerKey& other) const;
 
+  bool valid() const;
+
   bool operator==(const LayerKey& other) const;
 
   inline bool operator!=(const LayerKey& other) const {
     return !this->operator==(other);
   }
-
-  inline operator bool() const { return layer != UNKNOWN_LAYER; }
 };
 
 std::ostream& operator<<(std::ostream& out, const LayerKey& key);
diff --git a/src/dynamic_scene_graph.cpp b/src/dynamic_scene_graph.cpp
index 4ff70ad..4cb04db 100644
--- a/src/dynamic_scene_graph.cpp
+++ b/src/dynamic_scene_graph.cpp
@@ -57,6 +57,12 @@ DynamicSceneGraph::LayerIds getDefaultLayerIds() {
 DynamicSceneGraph::DynamicSceneGraph() : DynamicSceneGraph(getDefaultLayerIds()) {}
 
 DynamicSceneGraph::DynamicSceneGraph(const LayerIds& layer_ids) : layer_ids(layer_ids) {
+  for (const auto layer_id : layer_ids) {
+    if (layer_id == DsgLayers::UNKNOWN) {
+      throw std::domain_error("cannot instantiate layer with unknown layer id!");
+    }
+  }
+
   if (layer_ids.empty()) {
     throw std::domain_error("scene graph cannot be initialized without layers");
   }
@@ -236,7 +242,7 @@ bool DynamicSceneGraph::insertEdge(NodeId source,
     return false;
   }
 
-  if (!source_key || !target_key) {
+  if (!source_key.valid() || !target_key.valid()) {
     return false;
   }
 
@@ -267,7 +273,7 @@ bool DynamicSceneGraph::insertParentEdge(NodeId source,
     return false;
   }
 
-  if (!source_key || !target_key) {
+  if (!source_key.valid() || !target_key.valid()) {
     return false;
   }
 
@@ -467,7 +473,7 @@ bool DynamicSceneGraph::removeEdge(NodeId source, NodeId target) {
     return false;
   }
 
-  if (!source_key || !target_key) {
+  if (!source_key.valid() || !target_key.valid()) {
     return false;
   }
 
diff --git a/src/layer_prefix.cpp b/src/layer_prefix.cpp
index e5357f4..96fb50a 100644
--- a/src/layer_prefix.cpp
+++ b/src/layer_prefix.cpp
@@ -39,13 +39,17 @@
 
 namespace spark_dsg {
 
-LayerKey::LayerKey() : layer(LayerKey::UNKNOWN_LAYER) {}
+LayerKey::LayerKey() : layer(DsgLayers::UNKNOWN) {}
 
 LayerKey::LayerKey(LayerId layer_id) : layer(layer_id) {}
 
 LayerKey::LayerKey(LayerId layer_id, uint32_t prefix)
     : layer(layer_id), prefix(prefix), dynamic(true) {}
 
+bool LayerKey::isParent(const LayerKey& other) const { return layer > other.layer; }
+
+bool LayerKey::valid() const { return layer != DsgLayers::UNKNOWN; }
+
 bool LayerKey::operator==(const LayerKey& other) const {
   if (dynamic != other.dynamic) {
     return false;
@@ -59,8 +63,6 @@ bool LayerKey::operator==(const LayerKey& other) const {
   return same_layer && prefix == other.prefix;
 }
 
-bool LayerKey::isParent(const LayerKey& other) const { return layer > other.layer; }
-
 std::ostream& operator<<(std::ostream& out, const LayerKey& key) {
   if (key.dynamic) {
     out << key.layer << "(" << key.prefix << ")";
diff --git a/src/scene_graph_utilities.cpp b/src/scene_graph_utilities.cpp
index a1f0cbe..64f126a 100644
--- a/src/scene_graph_utilities.cpp
+++ b/src/scene_graph_utilities.cpp
@@ -48,7 +48,7 @@ void getAncestorsOfLayer(const DynamicSceneGraph& graph,
     return;
   }
 
-  if (node->layer <= child_layer) {
+  if (node->layer <= child_layer.layer) {
     return;
   }
 
diff --git a/tests/utest_layer_prefix.cpp b/tests/utest_layer_prefix.cpp
index 09e2595..a9f753e 100644
--- a/tests/utest_layer_prefix.cpp
+++ b/tests/utest_layer_prefix.cpp
@@ -65,13 +65,6 @@ TEST(LayerKeyTests, TestIsParent) {
   EXPECT_FALSE(key6.isParent(key4));
 }
 
-TEST(LayerKeyTests, TestKeyTruthValues) {
-  EXPECT_FALSE(LayerKey());
-  EXPECT_TRUE(LayerKey(1));
-  EXPECT_TRUE(LayerKey(2, 0));
-  EXPECT_TRUE(LayerKey(0));
-}
-
 TEST(LayerPrefixTests, TestMatches) {
   LayerPrefix a('a');
   EXPECT_TRUE(a.matches(NodeSymbol('a', 0)));