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

Refactor functions cpp #135

Merged
merged 4 commits into from
Jul 14, 2017
Merged

Refactor functions cpp #135

merged 4 commits into from
Jul 14, 2017

Conversation

Karsten1987
Copy link
Contributor

@Karsten1987 Karsten1987 commented Jul 11, 2017

connects to #131

summary of commits:

1.) split functions.cpp into multiple files (git blame on each file still shows correct history)
2.) cleanup these files with only their respective necessary parts
3.) change CMakeLists.txt to compile all files
4.) change TypeSupport_impl.hpp to correctly compile template specifications as inline
5.) a little bit of style for functions

I'll leave it as it is for now, hoping on getting this PR merged quickly. Further improvements can thus be done in separate PRs.

@Karsten1987 Karsten1987 added the in progress Actively being worked on (Kanban column) label Jul 11, 2017
@Karsten1987 Karsten1987 self-assigned this Jul 11, 2017

extern "C"
{
extern const char * const ros_topic_prefix;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're in an extern group, do you need to extern the internal definitions? Not sure if this was there previously, but I think the internal extern's should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aren't these two different things? The extern "C" is responsible for symbol mangling and the extern const.. is for telling that the definition of this variable is happening in another file?

@Karsten1987
Copy link
Contributor Author

Karsten1987 commented Jul 12, 2017

Linux: Build Status
OSX: Build Status
Windows: Build Status
ARM: Build Status

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

The patch looks good to me. Since the PR should be merged without squashing at the end can you please squash some of the later commits wherever it makes sense (the first two / three commits must stay separated though).

Correctly compile inline functions
function style

include algorithm for demangle.cpp

fix typo

delete unused backup file
@dirk-thomas
Copy link
Member

Currently the first commit adds a .bck file and the last commit removes it again. I will rewrite the history on this branch to get rid of this temporary file.

@dirk-thomas
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas dirk-thomas merged commit a04cf6c into master Jul 14, 2017
@dirk-thomas dirk-thomas deleted the refactor_functions_cpp branch July 14, 2017 21:26
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Jul 14, 2017
@mikaelarguedas mikaelarguedas mentioned this pull request Aug 4, 2017
8 tasks
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.

3 participants