Mark code that should be unreachable (#77)

Introduce a new [[noreturn]] unreachable() function that marks code as unreachable and throws a logic error if it is executed.
Fix build error due to Windows min/max macros.
Fix linker errors from referring to a non-constexpr extern from a constexpr.
Fix warnings about narrowing conversions.

Signed-off-by: Dan Rose <dan@digilabs.io>

rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/exception.hpp

@@ -15,8 +15,8 @@
 #ifndef RMW_CYCLONEDDS_CPP__EXCEPTION_HPP_
 #define RMW_CYCLONEDDS_CPP__EXCEPTION_HPP_
 
+#include <stdexcept>
 #include <string>
-#include <exception>
 
 namespace rmw_cyclonedds_cpp
 {
@@ -35,6 +35,20 @@ protected:
   std::string m_message;
 };
 
+/// Stub for code that should never be reachable by design.
+/// If it is possible to reach the code due to bad data or other runtime conditions,
+/// use a runtime_error instead
+[[noreturn]] inline void unreachable()
+{
+#if defined(__has_builtin)
+#if __has_builtin(__builtin_unreachable)
+  __builtin_unreachable();
+#endif
+#elif (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5))
+  __builtin_unreachable();
+#endif
+  throw std::logic_error("This code should be unreachable.");
+}
 }  // namespace rmw_cyclonedds_cpp
 
 #endif  // RMW_CYCLONEDDS_CPP__EXCEPTION_HPP_

rmw_cyclonedds_cpp/src/Serialization.cpp

@@ -11,16 +11,22 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
+
+// suppress definition of min/max macros on Windows.
+// TODO(dan@digilabs.io): Move this closer to where Windows.h/Windef.h is included
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
 #include "Serialization.hpp"
 
-#include <algorithm>
+#include <array>
 #include <limits>
 #include <unordered_map>
 #include <vector>
 
 #include "TypeSupport2.hpp"
 #include "bytewise.hpp"
-#include "rmw_cyclonedds_cpp/TypeSupport_impl.hpp"
 
 namespace rmw_cyclonedds_cpp
 {
@@ -51,7 +57,7 @@ struct CDRCursor
     if (n_bytes == 1 || start_offset % n_bytes == 0) {
       return;
     }
-    advance((-start_offset) % n_bytes);
+    advance(n_bytes - start_offset % n_bytes);
     assert(offset() - start_offset < n_bytes);
     assert(offset() % n_bytes == 0);
   }
@@ -194,6 +200,8 @@ protected:
       case EncodingVersion::CDR1:
         eversion_byte = '\1';
         break;
+      default:
+        unreachable();
     }
     std::array<char, 4> rtps_header{eversion_byte,
       // encoding format = PLAIN_CDR
@@ -304,17 +312,21 @@ protected:
         case EValueType::SpanSequenceValueType:
         case EValueType::BoolVectorValueType:
           result = false;
+          break;
+        default:
+          unreachable();
       }
       trivially_serialized_cache.emplace(key, result);
     }
     return result;
   }
 
-  size_t get_cdr_alignof_primitive(ROSIDL_TypeKind vt)
+  size_t get_cdr_alignof_primitive(ROSIDL_TypeKind tk)
   {
     /// return 0 if the value type is not primitive
     /// else returns the number of bytes it should align to
-    return std::min(get_cdr_size_of_primitive(vt), max_align);
+    size_t sizeof_ = get_cdr_size_of_primitive(tk);
+    return sizeof_ < max_align ? sizeof_ : max_align;
   }
 
   void serialize(CDRCursor * cursor, const void * data, const PrimitiveValueType & value_type)
@@ -357,7 +369,8 @@ protected:
       case ROSIDL_TypeKind::STRING:
       case ROSIDL_TypeKind::WSTRING:
       case ROSIDL_TypeKind::MESSAGE:
-        throw std::logic_error("not a primitive");
+      default:
+        unreachable();
     }
   }
 
@@ -443,7 +456,7 @@ protected:
       if (auto s = dynamic_cast<const BoolVectorValueType *>(value_type)) {
         return serialize(cursor, data, *s);
       }
-      throw std::logic_error("Unhandled case");
+      unreachable();
     }
   }
 

rmw_cyclonedds_cpp/src/TypeSupport2.cpp

@@ -68,11 +68,13 @@ public:
 
 std::unique_ptr<StructValueType> make_message_value_type(const rosidl_message_type_support_t * mts)
 {
-  if (auto ts_c = mts->func(mts, TypeGeneratorInfo<TypeGenerator::ROSIDL_C>::identifier)) {
+  if (auto ts_c = mts->func(mts, TypeGeneratorInfo<TypeGenerator::ROSIDL_C>::get_identifier())) {
     auto members = static_cast<const MetaMessage<TypeGenerator::ROSIDL_C> *>(ts_c->data);
     return std::make_unique<ROSIDLC_StructValueType>(members);
   }
-  if (auto ts_cpp = mts->func(mts, TypeGeneratorInfo<TypeGenerator::ROSIDL_Cpp>::identifier)) {
+  if (auto ts_cpp =
+    mts->func(mts, TypeGeneratorInfo<TypeGenerator::ROSIDL_Cpp>::get_identifier()))
+  {
     auto members = static_cast<const MetaMessage<TypeGenerator::ROSIDL_Cpp> *>(ts_cpp->data);
     return std::make_unique<ROSIDLCPP_StructValueType>(members);
   }
@@ -83,7 +85,9 @@ std::unique_ptr<StructValueType> make_message_value_type(const rosidl_message_ty
 std::pair<std::unique_ptr<StructValueType>, std::unique_ptr<StructValueType>>
 make_request_response_value_types(const rosidl_service_type_support_t * svc_ts)
 {
-  if (auto tsc = svc_ts->func(svc_ts, TypeGeneratorInfo<TypeGenerator::ROSIDL_C>::identifier)) {
+  if (auto tsc =
+    svc_ts->func(svc_ts, TypeGeneratorInfo<TypeGenerator::ROSIDL_C>::get_identifier()))
+  {
     auto typed =
       static_cast<const TypeGeneratorInfo<TypeGenerator::ROSIDL_C>::MetaService *>(tsc->data);
     return {
@@ -93,7 +97,7 @@ make_request_response_value_types(const rosidl_service_type_support_t * svc_ts)
   }
 
   if (auto tscpp =
-    svc_ts->func(svc_ts, TypeGeneratorInfo<TypeGenerator::ROSIDL_Cpp>::identifier))
+    svc_ts->func(svc_ts, TypeGeneratorInfo<TypeGenerator::ROSIDL_Cpp>::get_identifier()))
   {
     auto typed =
       static_cast<const TypeGeneratorInfo<TypeGenerator::ROSIDL_Cpp>::MetaService *>(tscpp->data);

rmw_cyclonedds_cpp/src/TypeSupport2.hpp

@@ -15,6 +15,7 @@
 #define TYPESUPPORT2_HPP_
 
 #include <cassert>
+#include <functional>
 #include <memory>
 #include <regex>
 #include <string>
@@ -22,6 +23,7 @@
 #include <vector>
 
 #include "bytewise.hpp"
+#include "rmw_cyclonedds_cpp/exception.hpp"
 #include "rosidl_generator_c/string_functions.h"
 #include "rosidl_generator_c/u16string_functions.h"
 #include "rosidl_typesupport_introspection_c/identifier.h"
@@ -79,8 +81,7 @@ template<>
 struct TypeGeneratorInfo<TypeGenerator::ROSIDL_C>
 {
   static constexpr auto enum_value = TypeGenerator::ROSIDL_C;
-
+  static const auto & get_identifier() {return rosidl_typesupport_introspection_c__identifier;}
-  static constexpr auto & identifier = rosidl_typesupport_introspection_c__identifier;
   using MetaMessage = rosidl_typesupport_introspection_c__MessageMembers;
   using MetaMember = rosidl_typesupport_introspection_c__MessageMember;
   using MetaService = rosidl_typesupport_introspection_c__ServiceMembers;
@@ -90,7 +91,10 @@ template<>
 struct TypeGeneratorInfo<TypeGenerator::ROSIDL_Cpp>
 {
   static constexpr auto enum_value = TypeGenerator::ROSIDL_Cpp;
-  static constexpr auto & identifier = rosidl_typesupport_introspection_cpp::typesupport_identifier;
+  static const auto & get_identifier()
+  {
+    return rosidl_typesupport_introspection_cpp::typesupport_identifier;
+  }
   using MetaMessage = rosidl_typesupport_introspection_cpp::MessageMembers;
   using MetaMember = rosidl_typesupport_introspection_cpp::MessageMember;
   using MetaService = rosidl_typesupport_introspection_cpp::ServiceMembers;
@@ -327,9 +331,8 @@ struct PrimitiveValueType : public AnyValueType
       case ROSIDL_TypeKind::STRING:
       case ROSIDL_TypeKind::WSTRING:
       case ROSIDL_TypeKind::MESSAGE:
-        throw std::runtime_error(
+      default:
-                "not a primitive value type: " +
+        unreachable();
-                std::to_string(std::underlying_type_t<ROSIDL_TypeKind>(m_type_kind)));
     }
   }
   EValueType e_value_type() const override {return EValueType::PrimitiveValueType;}
@@ -482,6 +485,8 @@ auto AnyValueType::apply(UnaryFunction f) const
       return f(*static_cast<const SpanSequenceValueType *>(this));
     case EValueType::BoolVectorValueType:
       return f(*static_cast<const BoolVectorValueType *>(this));
+    default:
+      unreachable();
   }
 }
 
@@ -503,6 +508,8 @@ auto AnyValueType::apply(UnaryFunction f)
       return f(*static_cast<SpanSequenceValueType *>(this));
     case EValueType::BoolVectorValueType:
       return f(*static_cast<BoolVectorValueType *>(this));
+    default:
+      unreachable();
   }
 }
 

rmw_cyclonedds_cpp/src/bytewise.hpp

@@ -18,6 +18,9 @@
 
 #include "dds/ddsrt/endian.h"
 
+using std::size_t;
+using std::ptrdiff_t;
+
 enum class endian
 {
   little = DDSRT_LITTLE_ENDIAN,

rmw_cyclonedds_cpp/src/rmw_node.cpp

@@ -1031,6 +1031,8 @@ static dds_qos_t * create_readwrite_qos(
     case RMW_QOS_POLICY_HISTORY_KEEP_ALL:
       dds_qset_history(qos, DDS_HISTORY_KEEP_ALL, DDS_LENGTH_UNLIMITED);
       break;
+    default:
+      rmw_cyclonedds_cpp::unreachable();
   }
   switch (qos_policies->reliability) {
     case RMW_QOS_POLICY_RELIABILITY_SYSTEM_DEFAULT:
@@ -1041,6 +1043,8 @@ static dds_qos_t * create_readwrite_qos(
     case RMW_QOS_POLICY_RELIABILITY_BEST_EFFORT:
       dds_qset_reliability(qos, DDS_RELIABILITY_BEST_EFFORT, 0);
       break;
+    default:
+      rmw_cyclonedds_cpp::unreachable();
   }
   switch (qos_policies->durability) {
     case RMW_QOS_POLICY_DURABILITY_SYSTEM_DEFAULT:
@@ -1060,6 +1064,8 @@ static dds_qos_t * create_readwrite_qos(
           DDS_LENGTH_UNLIMITED, DDS_LENGTH_UNLIMITED);
         break;
       }
+    default:
+      rmw_cyclonedds_cpp::unreachable();
   }
   /* deadline, lifespan, liveliness are not yet supported */
   if (ignore_local_publications) {
@@ -1092,6 +1098,8 @@ static bool get_readwrite_qos(dds_entity_t handle, rmw_qos_profile_t * qos_polic
         qos_policies->history = RMW_QOS_POLICY_HISTORY_KEEP_ALL;
         qos_policies->depth = (uint32_t) depth;
         break;
+      default:
+        rmw_cyclonedds_cpp::unreachable();
     }
   }
 
@@ -1109,6 +1117,8 @@ static bool get_readwrite_qos(dds_entity_t handle, rmw_qos_profile_t * qos_polic
       case DDS_RELIABILITY_RELIABLE:
         qos_policies->reliability = RMW_QOS_POLICY_RELIABILITY_RELIABLE;
         break;
+      default:
+        rmw_cyclonedds_cpp::unreachable();
     }
   }
 
@@ -1129,6 +1139,8 @@ static bool get_readwrite_qos(dds_entity_t handle, rmw_qos_profile_t * qos_polic
       case DDS_DURABILITY_PERSISTENT:
         qos_policies->durability = RMW_QOS_POLICY_DURABILITY_UNKNOWN;
         break;
+      default:
+        rmw_cyclonedds_cpp::unreachable();
     }
   }
 
@@ -1176,6 +1188,8 @@ static bool get_readwrite_qos(dds_entity_t handle, rmw_qos_profile_t * qos_polic
       case DDS_LIVELINESS_MANUAL_BY_TOPIC:
         qos_policies->liveliness = RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC;
         break;
+      default:
+        rmw_cyclonedds_cpp::unreachable();
     }
     if (lease_duration == DDS_INFINITY) {
       qos_policies->liveliness_lease_duration.sec = qos_policies->liveliness_lease_duration.nsec =
@@ -1823,6 +1837,9 @@ extern "C" rmw_ret_t rmw_take_event(
     case RMW_EVENT_INVALID: {
         break;
       }
+
+    default:
+      rmw_cyclonedds_cpp::unreachable();
   }
   *taken = false;
   return RMW_RET_ERROR;

rmw_cyclonedds_cpp/src/serdata.cpp

@@ -111,7 +111,10 @@ void * create_response_type_support(
 
 static uint32_t serdata_rmw_size(const struct ddsi_serdata * dcmn)
 {
-  return static_cast<const serdata_rmw *>(dcmn)->size();
+  size_t size = static_cast<const serdata_rmw *>(dcmn)->size();
+  uint32_t size_u32(size);
+  assert(size == size_u32);
+  return size_u32;
 }
 
 static void serdata_rmw_free(struct ddsi_serdata * dcmn)
@@ -142,7 +145,7 @@ static struct ddsi_serdata * serdata_rmw_from_ser(
       memcpy(cursor, src, n_bytes);
       cursor = byte_offset(cursor, n_bytes);
       off = fragchain->maxp1;
-      assert(off < size);
+      assert(off <= size);
     }
     fragchain = fragchain->nextfrag;
   }
@@ -491,7 +494,7 @@ struct sertopic_rmw * create_sertopic(
   return st;
 }
 
-void serdata_rmw::resize(uint32_t requested_size)
+void serdata_rmw::resize(size_t requested_size)
 {
   if (!requested_size) {
     m_size = 0;

rmw_cyclonedds_cpp/src/serdata.hpp

@@ -43,15 +43,15 @@ struct sertopic_rmw : ddsi_sertopic
 class serdata_rmw : public ddsi_serdata
 {
 protected:
-  uint32_t m_size {0};
+  size_t m_size {0};
   /* first two bytes of data is CDR encoding
      second two bytes are encoding options */
   std::unique_ptr<byte[]> m_data {nullptr};
 
 public:
   serdata_rmw(const ddsi_sertopic * topic, ddsi_serdata_kind kind);
-  void resize(uint32_t requested_size);
+  void resize(size_t requested_size);
-  uint32_t size() const {return m_size;}
+  size_t size() const {return m_size;}
   void * data() const {return m_data.get();}
 };