Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

simplify code relaying all symbols #29

Merged
merged 7 commits into from
Oct 16, 2017
Merged

simplify code relaying all symbols #29

merged 7 commits into from
Oct 16, 2017

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Oct 12, 2017

This slightly updates #28 to:

  • update the macro names (underscore between name and number)

  • remove semicolons after macro calls

  • declare rmw_init explicitly to avoid the init_fn argument as well as the FOO_INIT macro

  • Linux Build Status

  • Linux-aarch64 Build Status

  • macOS Build Status

  • Windows Build Status

jwang11 and others added 2 commits October 9, 2017 15:52
The commit use classic c preprocessor approch to handle variadic parameter, which define
RMW interface implementation and handle ARG values inside RMW_INTERFACE_FN macro.
It obviously reduce code lines and complexity of rmw interface implementation.
@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Oct 12, 2017
@dirk-thomas dirk-thomas self-assigned this Oct 12, 2017
@dirk-thomas dirk-thomas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Oct 12, 2017
@jwang11
Copy link

jwang11 commented Oct 12, 2017

Looks windows build still fail, I'll check, BTW, are you using VS 2017 community ?

@dirk-thomas
Copy link
Member Author

BTW, are you using VS 2017 community ?

We are still using Visual Studio 2015.

@jwang11
Copy link

jwang11 commented Oct 12, 2017

find the reason, EXPAND should also apply to ARGS_ and ARG_VALUES, patch done

@dirk-thomas
Copy link
Member Author

@jwang11 Thank you. The new patch looks good. The ongoing Windows build looks good. I will merge this once it is green and the PR got approved.

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 13, 2017
@dirk-thomas
Copy link
Member Author

For now I have just suppressed the warning: 3d89670#diff-0e10897bf5aa86e527bbfbb0e10759e9R202

@dirk-thomas
Copy link
Member Author

Waiting for review.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually find this a bit more difficult to understand. I guess it makes it easier to change the calling mechanism, but I don't expect that to happen very often. Definitely requires more study before you can see what's going on though.

However, I didn't see anything technically wrong with it while reviewing, so lgtm.

}

void * symbol_rmw_init = nullptr;

rmw_ret_t
rmw_init(void)
rmw_ret_t rmw_init()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: why this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the first round all functions were refactored into macros and in a second step rmw_init was extracted again. I committed 7d4b932 to avoid the diff.

@dirk-thomas
Copy link
Member Author

I actually find this a bit more difficult to understand. I guess it makes it easier to change the calling mechanism, but I don't expect that to happen very often. Definitely requires more study before you can see what's going on though.

In my opinion the major advantage is the reduction by 260 lines of code as well as not duplicating the function signature of every rmw function anymore.

@dirk-thomas dirk-thomas merged commit afae8f8 into master Oct 16, 2017
@dirk-thomas dirk-thomas deleted the pr28 branch October 16, 2017 19:04
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants