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

use iterators for split #14

Merged
merged 2 commits into from
Jun 13, 2019
Merged

use iterators for split #14

merged 2 commits into from
Jun 13, 2019

Conversation

Karsten1987
Copy link
Contributor

I am extending the current split implementation for a iterator based design. This allows to use the same functionality for split to be used in different containers (e.g. std::list, std::set) and gives the opportunity for custom iterators. Especially the latter seems quite useful in order to correctly split and parse a message definition and extract it as a tuple.

CI: (build: up-to rcpputils, test: packages-select rcpputils)

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

Signed-off-by: Karsten Knese karsten@openrobotics.org

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987 Karsten1987 requested a review from a team as a code owner June 11, 2019 16:48
@Karsten1987 Karsten1987 self-assigned this Jun 11, 2019
Copy link

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

mainly LGTM, a few nits

split(const std::string & input, char delim, bool skip_empty = false)
template<
class InsertIterator,
typename std::enable_if<

Choose a reason for hiding this comment

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

Could static_assert be used here instead to improve readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I opted for SFINAE is that static_assert doesn't participate in function overloading. That being, the current template approach has the benefit of having another function declaration set with std::enable_if which accepts a different form of iterator. One of them could be just a class member function, e.g. add which takes the string and operates on it.
Does this make sense?

Choose a reason for hiding this comment

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

I see, that makes sense. I just feel it's hard to read and I wished there was a better way to write this, but I don't see any right now so this LGTM ;)

include/rcpputils/split.hpp Outdated Show resolved Hide resolved
test/test_split.cpp Outdated Show resolved Hide resolved
test/test_split.cpp Show resolved Hide resolved
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

I hopefully addressed all your comments. I am happy to re-iterate over static_assert vs SFINAE. I don't necessarily feel super strong about it.

@emersonknapp emersonknapp merged commit 398d5bf into master Jun 13, 2019
@delete-merged-branch delete-merged-branch bot deleted the generic_split branch June 13, 2019 18:32
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.

4 participants