Replace message assertion with logging in order to have release modes…#2096
Conversation
650a66d to
3a800c4
Compare
| ROS_DEBUG("Trying to publish message of type [%s/%s] on a " | ||
| "publisher with type [%s/%s]", | ||
| mt::datatype<M>(*message), mt::md5sum<M>(*message), | ||
| impl_->datatype_.c_str(), impl_->md5sum_.c_str()); |
There was a problem hiding this comment.
How about adding if statement to check message type, then call ROS_FATAL and ROS_BREAK? i think this is fatal information since it does not publish the correct message as user application expects.
There was a problem hiding this comment.
With this, it won't even compile in case of such a type mismatch. ROS_DEBUG is just a macro that I'm sure of not being opt-out by the compiler in release mode 🙂 Any similar line could be replaced with it. The important bits are mt::datatype<M>(*message) and mt::md5sum<M>(*message), which fail when type mismatch occurs.
I mean, replacements with ROS_FATAL or ROS_BREAK won't compile either. Ultimately, any mismatches in the published messages will be captured in compile-time.
There was a problem hiding this comment.
ROS_DEBUG is just a macro that I'm sure of not being opt-out by the compiler in release mode
i do not think so, it actually depends on build environment.
in default, it does not opt-out ROS_DEBUG nor ROS_FATAL. (So that we can see the ROS_INFO with release mode)
ROS_BREAK is the same with ROS_ASSERT_MSG, which i think it would be nice to abort when it is built with debug mode.
The important bits are mt::datatype(*message) and mt::md5sum(*message), which fail when type mismatch occurs.
that is exactly why i was thinking that using if statement to check impl_->md5sum_ == "*" || std::string(mt::md5sum<M>(*message)) == "*" || impl_->md5sum_ == mt::md5sum<M>(*message) then print message. and with debug built it goes abort.
There was a problem hiding this comment.
So, do you suggest guarding ROS_DEBUG/ROS_BREAK statement with an if-check, which would be never opt-out?
There was a problem hiding this comment.
Yes. ROS_BREAK will be opt-out with release build as i mentioned previous comment. but as far as I see, this seems to be a fatal problem for user application because message cannot be delivered to the other side. of course this is just my idea, so I'd like to hear from others.
There was a problem hiding this comment.
If we guard with if-check, such delivery is irrelevant in here as it will never compile in release mode, too. I'm trying to make sure that the mismatches in delivery are noticed in compile-time.
There was a problem hiding this comment.
okay, i see what you mean. type mismatch will be checked during compile time, then just prints the debug information if user configures loglevel into debug. that makes sense. thanks for iterating 👍
| "Trying to publish message of type [%s/%s] on a publisher with type [%s/%s]", | ||
| mt::datatype<M>(message), mt::md5sum<M>(message), | ||
| impl_->datatype_.c_str(), impl_->md5sum_.c_str()); | ||
| ROS_DEBUG("Trying to publish message of type [%s/%s] on a " |
4535f88 to
bc44432
Compare
… to fail in compilation when msg type mismatches occur.
bc44432 to
d99022e
Compare
|
@sloretz You seem to be the latest maintainer. Could you please have a look? |
jacobperron
left a comment
There was a problem hiding this comment.
The change LGTM. Cheers!
… to fail in compilation when msg type mismatches occur (#2096)
… to fail in compilation when msg type mismatches occur.
Fixes #2095.