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

Add rcutils_string_array_sort function #248

Merged
merged 9 commits into from
Jun 11, 2020
Merged

Add rcutils_string_array_sort function #248

merged 9 commits into from
Jun 11, 2020

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented May 13, 2020

This change adds a simple and safe mechanism to order the entries in a string_array object lexicographically.

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

This change adds a simple mechanism to order the entries in a
`string_array` object lexicographically.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay added enhancement New feature or request in review Waiting for review (Kanban column) labels May 13, 2020
@cottsay cottsay self-assigned this May 13, 2020
Copy link
Contributor

@Blast545 Blast545 left a comment

Choose a reason for hiding this comment

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

I think the strdup part is important and should be addressed

test/test_string_array.cpp Outdated Show resolved Hide resolved
test/test_string_array.cpp Outdated Show resolved Hide resolved
test/test_string_array.cpp Show resolved Hide resolved
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>

Co-authored-by: Jorge Perez <jjperez@ekumenlabs.com>
Blast545
Blast545 previously approved these changes May 15, 2020
Signed-off-by: Scott K Logan <logans@cottsay.net>
@Karsten1987
Copy link
Contributor

I know, I am late to the party here, but would it make sense to extract the actual sort function qsort into its own function and expose the appropriate API to it? Why I am saying this, is because we have quite a handful of different arrays which could technically all profit from such a sorting function. All the individual arrays had to provide then would be a compare function.

@cottsay
Copy link
Member Author

cottsay commented Jun 8, 2020

...extract the actual sort function qsort into its own function and expose the appropriate API to it

Thanks for your interest, @Karsten1987. Could you elaborate on what that might look like? Are you looking for a re-implementation of qsort, or an adapter to make it easier to use with the rcutils arrays?

Signed-off-by: Scott K Logan <logans@cottsay.net>
@Karsten1987
Copy link
Contributor

no, no, please don't reimplement qsort ;-)

What I was trying to say is that if you look at the interface to qsort you'll notice that it's dealing with void *, meaning it's agnostic to its data type. So we could extract that same interface in its own function within rcutils, such as rcutils_qsort or similar, which deals with error checking in a way we'd like it, but basically only wraps qsort.

With that, all different array types could be used simply by providing a tailored compare function. That way, all we can reuse the same sort function for all types:

ret_t rcutils_qsort(string_array, string_array.size, sizeof(char *), &compare_string_array);
ret_t rcutils_qsort(uint8_array,  uint8_array.size, sizeof(uint8_t), &compare_uint8_array)
...

does this make sense?

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay marked this pull request as draft June 9, 2020 00:59
@cottsay
Copy link
Member Author

cottsay commented Jun 9, 2020

Alright @Karsten1987, I took a stab at this, but because of the argument checking, it doesn't come out as clean as I'd like it to. Please have a look and let me know if you see a better way.

@Karsten1987
Copy link
Contributor

I like it quite a bit. I think that makes it really valuable also for other array types 👍

cottsay added 2 commits June 10, 2020 16:04
Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay marked this pull request as ready for review June 10, 2020 23:21
@cottsay
Copy link
Member Author

cottsay commented Jun 10, 2020

This PR has changed quite a bit. Resetting reviews...

@cottsay cottsay dismissed Blast545’s stale review June 10, 2020 23:23

Lots of changes since review

@cottsay cottsay requested a review from Karsten1987 June 10, 2020 23:23
Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

two little nits. Looks good to me.

include/rcutils/types/string_array.h Outdated Show resolved Hide resolved
include/rcutils/types/string_array.h Outdated Show resolved Hide resolved
Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Member Author

cottsay commented Jun 11, 2020

Keen eye, @Karsten1987. I addressed your feedback in f93dc6a. Thanks for reviewing.

@cottsay cottsay merged commit 8c1d190 into master Jun 11, 2020
@delete-merged-branch delete-merged-branch bot deleted the sort_string_array branch June 11, 2020 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants