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.

Sign up for GitHub

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

Jump to bottom

introduce rcutils_strcasecmp, case insensitive string compare. #280

Merged
merged 5 commits into from
Sep 4, 2020
Merged

introduce rcutils_strcasecmp, case insensitive string compare. #280

merged 5 commits into from
Sep 4, 2020

Conversation

fujitatomoya
Copy link
Collaborator

close #279

Signed-off-by: Tomoya.Fujita Tomoya.Fujita@sony.com

@fujitatomoya fujitatomoya mentioned this pull request Aug 23, 2020
Closed
@fujitatomoya
Copy link
Collaborator Author

@iuhilnehc-ynos @Ada-King
CC: @clalancette

either of you, could you take a look at this? it seems that ros2/rcl#741 does not require this anymore, but i guess that this would be useful.

iuhilnehc-ynos
iuhilnehc-ynos reviewed Aug 27, 2020
View reviewed changes
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

@fujitatomoya

nitpick, just some minor comments.

CMakeLists.txt Outdated Show resolved Hide resolved
include/rcutils/strcasecmp.h Outdated Show resolved Hide resolved
src/strcasecmp.c Show resolved Hide resolved
include/rcutils/strcasecmp.h Show resolved Hide resolved
test/test_strcasecmp.cpp Outdated Show resolved Hide resolved
@fujitatomoya fujitatomoya force-pushed the feature-20200823-case-insensitive-cmp branch from 2f949db to 5a6a26c Compare August 28, 2020 07:47
@fujitatomoya
Copy link
Collaborator Author

@iuhilnehc-ynos @Ada-King

comments are addressed, could you check when you got time?

clalancette
clalancette requested changes Aug 31, 2020
View reviewed changes
include/rcutils/strcasecmp.h Outdated Show resolved Hide resolved
include/rcutils/strcasecmp.h Outdated Show resolved Hide resolved
src/strcasecmp.c Show resolved Hide resolved
test/test_strcasecmp.cpp Outdated Show resolved Hide resolved
@fujitatomoya
introduce rcutils_strcasecmp, case insensitive string compare.
35027fa 
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
add rcutils_strncasecmp with test.
3a2e1d4 
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya force-pushed the feature-20200823-case-insensitive-cmp branch from 66e0cb3 to 3a2e1d4 Compare September 2, 2020 02:10
@fujitatomoya
add comparison result value, update test to check if less or greater …
f07f00e 
…than zero.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@clalancette @iuhilnehc-ynos @Ada-King

could you do review again?

clalancette
clalancette requested changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A couple more things to fix, then this will be ready to have CI.

include/rcutils/strcasecmp.h Outdated Show resolved Hide resolved
include/rcutils/strcasecmp.h Outdated Show resolved Hide resolved
include/rcutils/strcasecmp.h Outdated Show resolved Hide resolved
include/rcutils/strcasecmp.h Outdated Show resolved Hide resolved
src/strcasecmp.c Outdated Show resolved Hide resolved
src/strcasecmp.c Outdated Show resolved Hide resolved
@fujitatomoya
always return 0 if method is successfully called.
20b9c98 
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@clalancette

thanks for the suggestion, i changed it back along with strcasecmp behavior. could you check when you got time?

@clalancette
Copy link
Contributor

clalancette commented Sep 3, 2020 •
edited
Loading

CI (only up to rcutils since this is a new API):

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

@clalancette
Copy link
Contributor

@fujitatomoya It looks like Windows has some warnings with this change. Can you look into it? Thanks.

@fujitatomoya
Copy link
Collaborator Author

@clalancette

got it, i will address them in today. will get back to you.

@fujitatomoya
windows unhappy, use ISO C and C++ conformant name instead.
9b586f2 
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@clalancette

i believe windows is now comfortable with 9b586f2. Could you run CI again? thank!

@clalancette
Copy link
Contributor

Thanks. Here's another CI run:

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

@clalancette
Copy link
Contributor

Looks good now, thanks for iterating @fujitatomoya !

@clalancette clalancette merged commit a935f27 into ros2:master Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ada-King Ada-King Ada-King approved these changes

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos approved these changes

@clalancette clalancette clalancette approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Case insensitive string compare function.
4 participants
@fujitatomoya @clalancette @iuhilnehc-ynos @Ada-King