Static type support for rosidl_typesupport_c #110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pablogs9
force-pushed
the
feature/static_typesupport
branch
9 times, most recently
from
7ba5ffc to
897c820
Compare
clalancette
reviewed
May 14, 2021
There was a problem hiding this comment.
In general, I think this would be a fine feature to have.
I do have some concerns about the implementation. In no particular order:
- The
#ifdefin type_support_dispatch.hpp looks pretty ugly. It seems to me that it would be better to split the code underneath the#ifdefinto a separate file (dynamic_support_dispatch.hppor something), and then just call out to it as appropriate. - The new flag to CMakeLists.txt (
ROSIDL_TYPESUPPORT_STATIC_TYPESUPPORT) look somewhat redundant with the${rosidl_typesupport_c_LIBRARY_TYPE}" STREQUAL "STATIC"code. Is there any way to combine that logic together? - Similarly, the logic in the empy files feels like it would apply in both the old "STATIC" case and the new "ROSIDL_TYPESUPPORT_STATIC_TYPESUPPORT" case, but I may be misunderstanding something here.
@sloretz any thoughts here?
Signed-off-by: Your Name <[email protected]> Default to OFF Signed-off-by: Your Name <[email protected]> Linter Signed-off-by: Your Name <[email protected]> Fix Signed-off-by: Your Name <[email protected]> Refactor Reafactor Signed-off-by: Your Name <[email protected]> Lint Signed-off-by: Your Name <[email protected]>
pablogs9
force-pushed
the
feature/static_typesupport
branch
6 times, most recently
from
dca8e1e to
cab5a95
Compare
Signed-off-by: Your Name <[email protected]> Inferring flags from build type Signed-off-by: Your Name <[email protected]> Remove extra include Signed-off-by: Your Name <[email protected]> Update Fix rebase Signed-off-by: Your Name <[email protected]> Fixes Add messages Update Fix Fix Messages Update Linter Lint Remove messages Signed-off-by: Your Name <[email protected]>
pablogs9
force-pushed
the
feature/static_typesupport
branch
from
cab5a95 to
cdbd44d
Compare
|
Hello @clalancette, thanks a lot for your comments. I'm answering here:
|
Signed-off-by: Your Name <[email protected]>
pablogs9
force-pushed
the
feature/static_typesupport
branch
from
05738c1 to
be9c031
Compare
|
@clalancette Could you please recheck this PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello @sloretz and @clalancette,
here at micro-ROS, we are wondering if it is possible to implement the changes proposed here to allow
rosidl_typesupport_cto have some kind of statically linked typesupport dispatch for those environments where dynamic library are not allowed (embedded).Can we discuss this feature here? What are your opinions?
CC: @ralph-lange @JanStaschulat