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

Resolves #33: Remove Boost, use C++11 #36

Merged
merged 3 commits into from
Apr 11, 2018

Conversation

IanTheEngineer
Copy link
Contributor

@IanTheEngineer IanTheEngineer commented Mar 29, 2018

It may make sense to branch to melodic-devel prior to adding this commit.

  • Replaced boost::trim with C++11 lambda implementation of string trimming
  • Replaced all boost::shared_ptr with std::shared_ptr
    These changes were tested with all of the catkin tests provided, and passed.

@IanTheEngineer IanTheEngineer changed the title Resolves #33: Remove boost, use C++11 Resolves #33: Remove Boost, use C++11 Mar 29, 2018
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I created a branch melodic-devel. Can you, please, re-submit this PR targeting melodic-devel?

@v4hn v4hn changed the base branch from kinetic-devel to melodic-devel April 5, 2018 17:33
Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

@rhaschke I retargeted the request. Github allows for that since a year or so ✨

This is overdue and should have been in kinetic already. Thanks @IanTheEngineer !
I proposed a minor change.

@@ -0,0 +1,60 @@
// Copyright (c) 2017 Evan Teran
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this header should be in the public interface of the package.
I propose to either put it into an anonymous namespace in src/model.cpp (it's used only there atm)
or just move the file to src/ to avoid installing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this shouldn't be in the public interface. I'll move the file to the src/ directory as you and @rhaschke recommend.

@rhaschke
Copy link
Contributor

rhaschke commented Apr 5, 2018

... or just move the file to src/ to avoid installing it

I prefer this as it facilitates later reuse in another context.

- Replaced boost::trim with C++11 lambda implementation
  of string trimming
- Replaced all boost::shared_ptr with std::shared_ptr
src/trim.h Outdated
// String-Trimming Lambdas
// Author: Evan Teran / evan-teran
// URL: https://stackoverflow.com/a/217605
// MIT License Derived from Stack Overflow Policy:
Copy link
Contributor Author

@IanTheEngineer IanTheEngineer Apr 9, 2018

Choose a reason for hiding this comment

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

It appears I am mistaken on which license is required for code snippets from Stack Overflow. The whole discussion is rather heated and frankly, confusing, but it appears the license should be CC BY-SA 3.0. I believe BSD and CC are still compatible licenses, so I'll just update the license to CC.

@rhaschke
Copy link
Contributor

Hi @IanTheEngineer,
I'm wondering why you want to get rid of boost::trim at all? I totally agree that it makes sense to switch from boost::pointers to std::pointers. However, why should we not re-use other boost utility functions? I don't see the point of inserting header-only code (like trim) all around, instead of using, well-maintained boost code, also header-only.
@v4hn, @davetcoleman What should be our policy regarding boost code?

@IanTheEngineer
Copy link
Contributor Author

IanTheEngineer commented Apr 10, 2018

@rhaschke I don't have a particularly strong opinion on using boost::trim or not. The goal was just to use the tools afforded by C++11 where possible. These lambdas are pretty straightforward replacements for trim, but if we decide to leave boost as a build dependency just for trim, that's entirely acceptable.

@IanTheEngineer
Copy link
Contributor Author

@rhaschke if things looks good, I can squash everything down into one commit.

@davetcoleman
Copy link
Member

I agree boost is preferred over custom code, but should be replaced where possible by std

Thanks @IanTheEngineer !

@davetcoleman davetcoleman merged commit 7deee8c into moveit:melodic-devel Apr 11, 2018
@v4hn
Copy link
Contributor

v4hn commented Apr 11, 2018 via email

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

4 participants