change generators to IDL-based pipeline #11
Conversation
dirk-thomas
added
in review
dirk-thomas
force-pushed
the
idl-stage-7
branch
from
7116703 to
f41c7c5
Compare
dirk-thomas
force-pushed
the
idl-stage-7
branch
from
5c01ffe to
2af78ba
Compare
dirk-thomas
force-pushed
the
idl-stage-7
branch
2 times, most recently
from
cacdc90 to
f3af6c3
Compare
dirk-thomas
added
in progress
dirk-thomas
changed the title
dirk-thomas
force-pushed
the
idl-stage-7
branch
from
83f1901 to
c3a901b
Compare
dirk-thomas
added
in review
|
@sloretz Back in review. |
rosidl_typesupport_opensplice_c/cmake/rosidl_typesupport_opensplice_c_generate_interfaces.cmake
Show resolved
Hide resolved
rosidl_typesupport_opensplice_c/cmake/rosidl_typesupport_opensplice_c_generate_interfaces.cmake
Show resolved
Hide resolved
rosidl_typesupport_opensplice_c/resource/idl__dds_opensplice__type_support.c.em
Show resolved
Hide resolved
rosidl_typesupport_opensplice_c/resource/idl__rosidl_typesupport_opensplice_c.h.em
Show resolved
Hide resolved
rosidl_typesupport_opensplice_c/resource/msg__rosidl_typesupport_opensplice_c.h.em
Show resolved
Hide resolved
rosidl_typesupport_opensplice_c/cmake/rosidl_typesupport_opensplice_c_generate_interfaces.cmake
Show resolved
Hide resolved
@sloretz Can you please finish the review soonish. |
There was a problem hiding this comment.
made it through rosidl_typesupport_opensplice_c/resource/msg__type_support_c.cpp.em
@sloretz Can you please finish the review soonish.
I can change to just skimming code to make sure it looks roughly like the old code, and rely on time and automated testing to catch issues instead.
Realistically I can't review this like normal and finish it in the time I think you're looking for. I've spent a bunch of time reading the generated message code and the rosidl PR to figure out how this generator fits in, and I don't have many comments to show for it. More comments in code (especially in CMake code), and longer doc strings in rosidl_parser would have helped a lot.
rosidl_typesupport_opensplice_c/resource/msg__type_support_c.cpp.em
Outdated
Show resolved
Hide resolved
rosidl_typesupport_opensplice_c/resource/msg__type_support_c.cpp.em
Outdated
Show resolved
Hide resolved
rosidl_typesupport_opensplice_c/resource/msg__type_support_c.cpp.em
Outdated
Show resolved
Hide resolved
| elif isinstance(member.type.basetype, WString): | ||
| keys.add('rosidl_generator_c/u16string.h') | ||
| keys.add('rosidl_generator_c/u16string_functions.h') | ||
| elif isinstance(member.type.basetype, NamespacedType): |
There was a problem hiding this comment.
It looks like this adds imports for other messages if there is an array or sequence of messages, but where are these imports being added when member.type is a single NamespacedType?
There was a problem hiding this comment.
Only for nested types we need the header to be included. For plain namespaced type fields we only need the forward declared type support functions (starting around line 89).
There was a problem hiding this comment.
I think this is actually causing the linker warning on Windows. I changed the logic in 3d5f30d to include the header for not-nested namespaced types as well as fixed a bug in the forward declaration logic.
To clarify my timeline goal: since currently CI is green (minus linker warnings on Windows for OpenSplice) I would rather get the current state in than wait until WString is fully implemented / tested. All other 13 PRs have been approved so I am looking to get this one reviewed by Monday (?) in order to move forward with the merge. |
…ansion of message templates
rosidl_typesupport_opensplice_cpp/resource/msg__rosidl_typesupport_opensplice_cpp.hpp.em
Show resolved
Hide resolved
rosidl_typesupport_opensplice_cpp/resource/srv__rosidl_typesupport_opensplice_cpp.hpp.em
Outdated
Show resolved
Hide resolved
rosidl_typesupport_opensplice_cpp/resource/msg__type_support.cpp.em
Outdated
Show resolved
Hide resolved
rosidl_typesupport_opensplice_c/resource/msg__type_support_c.cpp.em
Outdated
Show resolved
Hide resolved
Connected to ros2/rosidl#334.