add array support #119
add array support #119
Conversation
| foreach(_pkg_name ${rosidl_generate_interfaces_DEPENDENCY_PACKAGE_NAMES}) | ||
| ament_target_dependencies(${_target_name} | ||
| ${_pkg_name} | ||
| "_${_pkg_name}__${_typesupport_impl}${PythonExtra_EXTENSION_SUFFIX}" |
There was a problem hiding this comment.
Unless I'm mistaken the arguments to ament_target_dependencies must be a package (which has been find_package'ed): https://github.com/ament/ament_cmake/blob/c1d3004db7cb33ded7aed38e00c9a9d888986309/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake#L15-L29
So putting what I believe is a library name here doesn't make sense to me. Maybe you meant to pass this library name to target_link_libraries for _target_name?
There was a problem hiding this comment.
Running the code should also result in this error message: https://github.com/ament/ament_cmake/blob/c1d3004db7cb33ded7aed38e00c9a9d888986309/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake#L40
There was a problem hiding this comment.
Thanks for noticing it! I think it's actually an artefact from when I was trying to link the generated libraries together I must have forgotten to remove it.
Surprisingly I didnt have an error when running cmake.
To avoid duplicating work on your side, could you comment on this PR #113 ?
All the cmake stuff has been changed there and the current PR is just rebased on top of #113.
So once #113 is reviewed, reviewing #119 is going to be much more straight forward.
There was a problem hiding this comment.
Please try to add debug message in there to check why you don't get an error message. It would be good to ensure that the API works as intended even if you don't need it for this change.
mikaelarguedas
force-pushed
the
array_support
branch
2 times, most recently
from
42166e3
to
cacb782
Compare
|
Now compiles on windows but rclpy tests are failing. http://ci.ros2.org/job/ci_windows/1263/ |
mikaelarguedas
force-pushed
the
array_support
branch
3 times, most recently
from
ec3a3f9
to
49d1a12
Compare
|
http://ci.ros2.org/job/ci_osx/986/ |
mikaelarguedas
force-pushed
the
array_support
branch
2 times, most recently
from
c2e4337
to
76e8396
Compare
mikaelarguedas
force-pushed
the
array_support
branch
from
76e8396
to
8ccf2f1
Compare
mikaelarguedas
added
in review
|
The type handling code is pretty tough to read but it looks good to me. |
| } | ||
| @[ if field.type.array_size is None]@ | ||
| if (!@(nested_type)__Array__init(&(ros_message->@(field.name)), size@(field.name))) { | ||
| assert(false && "unable to create array ros_message"); |
There was a problem hiding this comment.
This probably shouldn't use an assert, since it will not be checked in Release mode.
|
Other than auditing your use of What's the test coverage like on these new changes? Do we have test messages in place for this and are we exercising them on this code? |
|
I guess ros2/rclpy#23 adds testing of this. |
mikaelarguedas
force-pushed
the
array_support
branch
from
1207483
to
4c4d132
Compare
|
Done, now raising |
|
lgtm |
Connects to #114
Relies on #113