-
Notifications
You must be signed in to change notification settings - Fork 101
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
Conversation
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>
There was a problem hiding this 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
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>
Signed-off-by: Scott K Logan <logans@cottsay.net>
I know, I am late to the party here, but would it make sense to extract the actual sort function |
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>
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 With that, all different array types could be used simply by providing a tailored 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>
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. |
I like it quite a bit. I think that makes it really valuable also for other array types 👍 |
Signed-off-by: Scott K Logan <logans@cottsay.net>
This PR has changed quite a bit. Resetting reviews... |
There was a problem hiding this 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.
Signed-off-by: Scott K Logan <logans@cottsay.net>
Keen eye, @Karsten1987. I addressed your feedback in f93dc6a. Thanks for reviewing. |
This change adds a simple and safe mechanism to order the entries in a
string_array
object lexicographically.