fragile code

[ibtaylor]

These builtin types are not given actual values like (BOOL=0,BYTE=1,...)

ros_type_introspection/include/ros_type_introspection/builtin_types.hpp

Lines 11 to 18 in 97e31ee

	enum BuiltinType {
	BOOL , BYTE, CHAR,
	UINT8, UINT16, UINT32, UINT64,
	INT8, INT16, INT32, INT64,
	FLOAT32, FLOAT64,
	TIME, DURATION,
	STRING, OTHER
	};

so changing the order in the enum can break other code (also a potential crash here is more likely since BuiltinType is not a type-safe enum)

ros_type_introspection/include/ros_type_introspection/builtin_types.hpp

Lines 20 to 33 in 97e31ee

	inline const char* toStr(const BuiltinType& c)
	{
	static const char* names[] =
	{
	"BOOL" , "BYTE", "CHAR",
	"UINT8", "UINT16", "UINT32", "UINT64",
	"INT8", "INT16", "INT32", "INT64",
	"FLOAT32", "FLOAT64",
	"TIME", "DURATION",
	"STRING", "OTHER"
	};
	return names[ static_cast<int>(c) ];
	}

ros_type_introspection/include/ros_type_introspection/builtin_types.hpp

Lines 41 to 48 in 81fd041

	const int BuiltinTypeSize[OTHER] = {
	1, 1, 1,
	1, 2, 4, 8,
	1, 2, 4, 8,
	4, 8,
	8, 8,
	-1
	};

maybe better to use runtime asserts, and an unordered map (instead of compares)

inline const char* toStr(const BuiltinType& c)
{
  switch (c) {
  case BOOL:     return "BOOL";
  case BYTE:     return "BYTE";
  case INT8:     return "INT8";
  case CHAR:     return "CHAR";
  case UINT8:    return "UINT8";
  case UINT16:   return "UINT16";
  case UINT32:   return "UINT32";
  case UINT64:   return "UINT64";
  case INT16:    return "INT16";
  case INT32:    return "INT32";
  case INT64:    return "INT64";
  case FLOAT32:  return "FLOAT32";
  case FLOAT64:  return "FLOAT64";
  case TIME:     return "TIME";
  case DURATION: return "DURATION";
  case STRING:   return "STRING";
  case OTHER:    return "OTHER";
  }
  LOG(FATAL) << "unsupported builtin type value:" << static_cast<int>(c);
}

// this should only be called if BuiltinType has a size, otherwise is a programmer error
inline std::size_t builtinSize(const BuiltinType c) {
  DCHECK(HasSize(c));
  switch (c) {
  case BOOL:
  case BYTE:
  case INT8:
  case CHAR:
  case UINT8:    return 8;
  case UINT16:
  case INT16:    return 16;
  case UINT32:
  case INT32:
  case FLOAT32:  return 32;
  case UINT64:
  case INT64:
  case FLOAT64:
  case TIME:
  case DURATION: return 64;
  case STRING:
  case OTHER:
  }
  LOG(FATAL) << "invalid call to builtinSize for type:" << toStr(c);
}

inline BuiltinType toBuiltinType(const std::string& s) {
  static std::unordered_map<std::string, BuiltinType> string_to_builtin_map {
    { "bool", BOOL },
    { "byte", BYTE },
    { "char", CHAR },
    { "uint8", UINT8 },
    { "uint16", UINT16 },
    { "uint32", UINT32 },
    { "uint64", UINT64 },
    { "int8", INT8 },
    { "int16", INT16 },
    { "int32", INT32 },
    { "int64", INT64 },
    { "float32", FLOAT32 },
    { "float64", FLOAT64 },
    { "time", TIME },
    { "duration", DURATION },
    { "string", STRING },
  };
  CHECK(!s.empty());
  auto it = string_to_builtin_map.find(s);
  return it == string_to_builtin_map.cend() ? OTHER : it->second;
}

[facontidavide]

Thanks for the suggestions.

I applied the changes to the new branch dev. This will be the "official branch I will use before releasing 1.0.

https://github.com/facontidavide/ros_type_introspection/tree/dev

[ibtaylor]

Whoops, did builtinSize wrong. Looks like you caught it.