Update the rcutils_find* APIs to return SIZE_MAX on invalid arguments. by clalancette · Pull Request #74 · ros2/rcutils

clalancette · 2017-11-21T15:40:41Z

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

fixes #58

CI:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

dhood

there's inconsistency when the string is valid but empty. In most cases string_length is returned if the delimiter is not found, but in the case of "" as the input string, SIZE_MAX is being returned when the delimiter is not found. from #58 it sounds like SIZE_MAX should be returned in all cases

dhood · 2017-11-21T19:34:13Z

src/find.c

 {
-  if (!str) {
-    return 0;
+  if (!str || strlen(str) == 0) {


I don't think this change makes sense for findn or find_lastn below because it accepts strings that aren't null terminated

Oh yeah, you are definitely right about that. That should be a check if string_length == 0. I'll fix that.

Nit: please use 0 == string_length when updating

dhood · 2017-11-21T19:44:30Z

btw if we update to return SIZE_MAX whenever the delimiter is not found, usage such as

rcutils/src/logging.c

Line 473 in 77a7e9e

size_t chars_to_start_delim = rcutils_find(str + i, token_start_delimiter);

will have to be updated

mikaelarguedas · 2017-11-21T19:52:27Z

Dismissing my review, and +1ing both @dhood's review about non-null terminated strings.

btw if we update to return SIZE_MAX whenever the delimiter is not found, usage such as

~~It looks to me that this returns SIZE_MAX only if the provided string is invalid. It will return the length of the string if it's not found so this code doesn't need to be updated.~~

~~But it raises the question of "Should it return SIZE_MAX in both the case where the input string is invalid and the one where the deimiter is not found?"~~

Covered by previous review

clalancette · 2017-11-21T19:56:09Z

there's inconsistency when the string is valid but empty. In most cases string_length is returned if the delimiter is not found, but in the case of "" as the input string, SIZE_MAX is being returned when the delimiter is not found. from #58 it sounds like SIZE_MAX should be returned in all cases

That was sort of intentional; I was trying to make it so that you could determine whether the inputs were valid (SIZE_MAX if not), and whether you found the delimiter (offset of the delimiter, or length of entire string if not). Now that you mention it, though, an "empty" string could be considered a special case of "can't find delimiter". I'll take another look and see what happens if we change to returning SIZE_MAX for can't find delimiter, in all cases.

btw if we update to return SIZE_MAX whenever the delimiter is not found, usage such as

rcutils/src/logging.c

Line 473 in 77a7e9e
size_t chars_to_start_delim = rcutils_find(str + i, token_start_delimiter);
will have to be updated

So I actually did audit these callers. I opted not to make any changes in both of those cases in src/logging.c, mostly because the case where it returns SIZE_MAX is handled already for both of them. In particular, in both cases there is an if statement dealing with where the size is larger than what is left, and they both do the "correct" thing in that case. Maybe I should add a comment in those places; what do you think?

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

In particular, have it go looking from the end, not the beginning, which will cause it to get out early once it has found something. Note that we have to have a separate check for the 0th element, since we can't do a <= 0 inside of the for loop with an unsigned variable. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2017-11-21T20:38:04Z

It looks like it is reasonable to use SIZE_MAX for all cases of "not found", so I've changed to that. Let me know what you think.

dhood · 2017-11-21T21:21:30Z

test/test_find.cpp

-  size_t ret0 = test_findn("", '/', 0, 0);
-  LOG((size_t)0, ret0);
+  size_t ret0 = test_findn("", '/', 0, SIZE_MAX);
+  LOG((size_t)SIZE_MAX, ret0);


the size_t casts are unnecessary here and below now yeah?

Yeah, for the SIZE_MAX checks, that's probably right. I'll make that fix and submit a CI just to be sure.

I wasn't aware of this but judging from the warnings on OS X SIZE_MAX may not fit into size_t.. this probably means we'll need to reconsider the return type of the find functions if we want to take this approach

I wasn't aware of this but judging from the warnings on OS X SIZE_MAX may not fit into size_t.. this probably means we'll need to reconsider the return type of the find functions if we want to take this approach

Arg, that is annoying. I'll do a little more investigating to see what I can find here.

I see what is going on. The warning that clang is throwing is very misleading. On x86_64 macOS, size_t is a long unsigned int, which is 64-bit unsigned. SIZE_MAX is an unsigned long long, which is also 64-bit unsigned. The problem is that the LOG statement is using %zu to print out the values, and an unsigned long long is not technically a size_t, even though they are compatible. The solution is to make sure we still cast SIZE_MAX to a size_t before passing it to LOG (like what was there before). I'll fix that, and put a comment in there for the next hapless soul who tries to remove it.

dhood · 2017-11-21T21:22:38Z

In particular, in both cases there is an if statement dealing with where the size is larger than what is left, and they both do the "correct" thing in that case.

Yep you're right (I think I'd remembered they'd have issues if we returned -1 when it was originally suggested, but they are indeed good as is)

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2017-11-22T15:03:05Z

Linux
Linux-aarch64
macOS
Windows

Otherwise, macOS complains. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2017-11-22T20:07:06Z

Another try on CI:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2017-11-27T17:39:08Z

Thanks everyone for the reviews. I'm going to merge this now.

Update the rcutils_find* APIs to return SIZE_MAX on invalid arguments.

27cdd19

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 21, 2017

tfoote approved these changes Nov 21, 2017

View reviewed changes

dhood suggested changes Nov 21, 2017

View reviewed changes

clalancette added 3 commits November 21, 2017 19:57

Use string_length on appropriate APIs.

566f553

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Switch to always returning SIZE_MAX for every case of "can't be found".

570d05f

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

dhood reviewed Nov 21, 2017

View reviewed changes

Remove unnecessary cast from SIZE_MAX.

3f1371c

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Make sure to cast SIZE_MAX to size_t in tests.

7a18f51

Otherwise, macOS complains. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette mentioned this pull request Nov 27, 2017

Make LOG macro behave like genuine code #76

Merged

dhood approved these changes Nov 27, 2017

View reviewed changes

clalancette merged commit 094f384 into master Nov 27, 2017

clalancette deleted the find-size-max branch November 27, 2017 17:39

clalancette removed the in review Waiting for review (Kanban column) label Nov 27, 2017

Conversation

clalancette commented Nov 21, 2017

Uh oh!

dhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhood commented Nov 21, 2017

Uh oh!

mikaelarguedas commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clalancette commented Nov 21, 2017

Uh oh!

clalancette commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhood commented Nov 21, 2017

Uh oh!

clalancette commented Nov 22, 2017

Uh oh!

clalancette commented Nov 22, 2017

Uh oh!

clalancette commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mikaelarguedas commented Nov 21, 2017 •

edited

Loading