Add missing BuiltinType::getType() overloads

facontidavide / ros_type_introspection

Deserialize ROS messages that are unknown at compilation time

MIT License

61 stars 30 forks source link

Add missing BuiltinType::getType() overloads #25

Closed janEbert closed 5 years ago

janEbert commented 5 years ago

Some types are not supported for extraction.

BuiltinType is missing two getType() functions, namely for BYTE and CHAR which currently resolve to OTHER (as they are not overloaded like the other types) which prevents the correct type comparison in Variant::extract().

This PR adds those overloads. However, some stuff might be debatable:

I overloaded BYTE using unsigned char. Another possibility is using the C++17 std::byte class which is not as backwards compatible but semantically nicer.
CHAR is overloaded using char. This makes semantic sense but has one quirk: char can be a signed char or unsigned char (the same type as BYTE, although distinct in the sense that it does not break the template). I cannot think of when it could be problematic but there may be cases.

facontidavide commented 5 years ago

Thank you, I don't know how I missed them ;)

facontidavide commented 5 years ago

Why you did this to me? :( How could you?

http://build.ros.org/job/Ldev__ros_type_introspection__ubuntu_xenial_amd64/41/changes

janEbert commented 5 years ago

Oh man, I am so sorry! :( I wrote this from a Windows machine on which I could not build.

Because it was so simple, I would not have imagined it would break the build. What could be the reason?

janEbert commented 5 years ago

Ah I see, the types unsigned char and uint8_t point to the same type.

In that case, would it be better to remove the BYTE type from BuiltinTypes? I'm not able to find a better solution. Or remove the BYTE overload again in case other stuff depended on it (which sounds like a messy solution)?

Again, I am so sorry I killed the master of all things!

facontidavide commented 5 years ago

No problem, I was kidding, I will fix it later.

Cheers

janEbert commented 5 years ago

Thanks, that's a bit reassuring, but, I still trashed your master... :) Because I feel very bad, I did remove all occurrences of BYTE in this commit. Although I understand if you cannot trust me anymore. ☺️

janEbert commented 5 years ago

Something I stumbled upon which might explain why you missed those values in the first place: In the Wiki entry for messages in section 2.1.1 it says that both char and byte are deprecated already and were aliases for int8 and uint8 respectively.

In my opinion, this speaks for removal as those were aliases and do not make sense with the current implementation.