Static type support for rosidl_typesupport_c#110
Open
pablogs9 wants to merge 4 commits intoros2:rollingfrom
Open
Static type support for rosidl_typesupport_c#110pablogs9 wants to merge 4 commits intoros2:rollingfrom
pablogs9 wants to merge 4 commits intoros2:rollingfrom
Conversation
pablogs9
force-pushed
the
feature/static_typesupport
branch
9 times, most recently
from
7ba5ffc to
897c820
Compare
clalancette
reviewed
May 14, 2021
Contributor
clalancette
left a comment
clalancette
left a comment
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 <you@example.com> Default to OFF Signed-off-by: Your Name <you@example.com> Linter Signed-off-by: Your Name <you@example.com> Fix Signed-off-by: Your Name <you@example.com> Refactor Reafactor Signed-off-by: Your Name <you@example.com> Lint Signed-off-by: Your Name <you@example.com>
pablogs9
force-pushed
the
feature/static_typesupport
branch
6 times, most recently
from
dca8e1e to
cab5a95
Compare
Signed-off-by: Your Name <you@example.com> Inferring flags from build type Signed-off-by: Your Name <you@example.com> Remove extra include Signed-off-by: Your Name <you@example.com> Update Fix rebase Signed-off-by: Your Name <you@example.com> Fixes Add messages Update Fix Fix Messages Update Linter Lint Remove messages Signed-off-by: Your Name <you@example.com>
pablogs9
force-pushed
the
feature/static_typesupport
branch
from
cab5a95 to
cdbd44d
Compare
Member
Author
|
Hello @clalancette, thanks a lot for your comments. I'm answering here:
|
Signed-off-by: Your Name <you@example.com>
pablogs9
force-pushed
the
feature/static_typesupport
branch
from
05738c1 to
be9c031
Compare
|
@clalancette Could you please recheck this PR? |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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