ros / roscpp_core

ros distribution sandbox
88 stars 116 forks source link

-Wconversion warnings in melodic #112

Closed anja-sheppard closed 5 years ago

anja-sheppard commented 5 years ago

I have the -Wconversion tag in my CMake (version 3.5), and it caught some warnings in ros/time.h. Nothing urgent, just wanted to pass it along in case you guys didn't compile with that flag. (Running ROS Melodic with gcc 7.4.0)

In file included from /opt/ros/melodic/include/ros/ros.h:38:0,
                 from /root/src/path_to_my_header.h:5,
                 from /root/src/path_to_my_test.cpp:2:
/opt/ros/melodic/include/ros/time.h: In instantiation of ‘T& ros::TimeBase<T, D>::fromSec(double) [with T = ros::Time; D = ros::Duration]’:
/opt/ros/melodic/include/ros/time.h:191:40:   required from here
/opt/ros/melodic/include/ros/time.h:161:11: warning: conversion to ‘uint32_t {aka unsigned int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
       sec += (nsec / 1000000000ul);
       ~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/opt/ros/melodic/include/ros/time.h: In instantiation of ‘T& ros::TimeBase<T, D>::fromSec(double) [with T = ros::WallTime; D = ros::WallDuration]’:
/opt/ros/melodic/include/ros/time.h:247:44:   required from here
/opt/ros/melodic/include/ros/time.h:161:11: warning: conversion to ‘uint32_t {aka unsigned int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
/opt/ros/melodic/include/ros/time.h: In instantiation of ‘T& ros::TimeBase<T, D>::fromSec(double) [with T = ros::SteadyTime; D = ros::WallDuration]’:
/opt/ros/melodic/include/ros/time.h:281:48:   required from here
/opt/ros/melodic/include/ros/time.h:161:11: warning: conversion to ‘uint32_t {aka unsigned int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
dirk-thomas commented 5 years ago

Please try to compile the latest code from the kinetic-devel branch which includes #106. That will change the referenced lines already.

anja-sheppard commented 5 years ago

Can I do that even though I'm using ROS Melodic?

dirk-thomas commented 5 years ago

Yes, the kinetic-devel branch is the default / latest branch and is used for all ROS distro after Kinetic too.

anja-sheppard commented 5 years ago

Ok thanks! I didn't know that.

I built rostime, but how do I make sure that my code is using the built time.h and not whatever is in roscpp_core? Sorry for all of the questions.

dirk-thomas commented 5 years ago

If you build your code in the same workspace together with this repo it should use the headers from this branch. Hopefully the warning will be gone then. If they are still there please check the path of the header to ensure it is using the branch and not the one from the system package.

anja-sheppard commented 5 years ago

Excellent, thanks. Built with this repo branch kinetic-devel, but still got one warning:

In file included from /opt/ros/melodic/include/ros/publisher.h:34:0,
                 from /opt/ros/melodic/include/ros/node_handle.h:32,
                 from /opt/ros/melodic/include/ros/ros.h:45,
                 from /root/src/my_ros_ws/src/item_stream_service_server.cpp:2:
/root/src/my_ros_ws/src/roscpp_core/roscpp_serialization/include/ros/serialization.h: In instantiation of ‘void ros::serialization::deserializeMessage(const ros::SerializedMessage&, M&) [with M = item_server::RequestItemStreamRequest_<std::allocator<void> >]’:
/opt/ros/melodic/include/ros/service_callback_helper.h:176:28:   required from ‘bool ros::ServiceCallbackHelperT<Spec>::call(ros::ServiceCallbackHelperCallParams&) [with Spec = ros::ServiceSpec<item_server::RequestItemStreamRequest_<std::allocator<void> >, item_server::RequestItemStreamResponse_<std::allocator<void> > >]’
/root/src/my_ros_ws/src/item_server/src/item_stream_service_server.cpp:26:1:   required from here
/root/src/my_ros_ws/src/roscpp_core/roscpp_serialization/include/ros/serialization.h:859:42: warning: conversion to ‘uint32_t {aka unsigned int}’ from ‘long unsigned int’ may alter its value [-Wconversion]
   IStream s(m.message_start, m.num_bytes - (m.message_start - m.buf.get()));
anja-sheppard commented 5 years ago

Fixed the problem - I'll start a pull request.

anja-sheppard commented 5 years ago

@dirk-thomas Any update on this?

dirk-thomas commented 5 years ago

Thanks for the ping. I just merged the PR.