Fix yaml parser error when meets .nan #741

Ada-King · 2020-08-12T03:36:08Z

Related issue 555

The problem has examined on my host machine.

Signed-off-by: Ada-King Bingtao.Du@sony.com

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

clalancette

Besides the inline comment, can you also please add a test for this case? Thanks.

rcl_yaml_param_parser/src/parser.c

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

Ada-King · 2020-08-18T02:57:58Z

Besides the inline comment, can you also please add a test for this case? Thanks.

Ada-King · 2020-08-18T03:05:08Z

Besides the inline comment, can you also please add a test for this case? Thanks.

Close this by accident... never mind.
The code has been modified according to your suggestion. Please check :)

rcl_yaml_param_parser/src/parser.c

rcl_yaml_param_parser/test/test_parse_yaml.cpp

rcl_yaml_param_parser/test/special_float2.yaml

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

clalancette

Thanks for iterating on this! This is exactly what I was looking for.

I have a few more minor comments inline, and this also needs to be rebased. Once that happens, I'll run CI on this.

rcl_yaml_param_parser/src/parser.c

rcl_yaml_param_parser/test/special_float.yaml

rcl_yaml_param_parser/test/test_parse_yaml.cpp

fujitatomoya

LGTM with @clalancette 's comments.

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

Ada-King · 2020-08-20T02:58:26Z

Thanks for iterating on this! This is exactly what I was looking for.

I have a few more minor comments inline, and this also needs to be rebased. Once that happens, I'll run CI on this.

I think this is ready to be merged, please has a re-review. Thanks.

clalancette

Looks good to me. I'm going to approve and run CI on it. Note that this is going to cause merge conflicts with #754 . @brawner FYI, let me know how you'd like to proceed here.

clalancette · 2020-08-20T12:16:28Z

CI:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2020-08-20T14:29:46Z

So, Windows is failing because it doesn't like strcasecmp. I'm not entirely sure how to solve that, but it needs further looking into.

fujitatomoya · 2020-08-20T15:13:45Z

what about using stricmp with case insensitivity instead?

brawner · 2020-08-20T17:51:39Z

It sounds like this PR can be merged as soon as it passes CI. #754 is not blocking anything at the moment, so I'm happy to rebase it on top of this PR.

clalancette · 2020-08-20T18:13:07Z

It sounds like this PR can be merged as soon as it passes CI. #754 is not blocking anything at the moment, so I'm happy to rebase it on top of this PR.

Sounds good, thanks @brawner

Ada-King · 2020-08-21T02:50:40Z

@clalancette @fujitatomoya
Maybe we can define a macro for the windows platform at the beginning of the parser.c file

#ifdef _WIN32
#define strcasecmp stricmp
#endif

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

fujitatomoya · 2020-08-21T06:25:06Z

rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h

+#ifdef _WIN32
+#define strcasecmp stricmp
+#endif
+


I think that there is no standard case insensitive string compare in C. but I am negative on this, this will cause complication and dependency especially for maintenance. (at least i believe that this is not the right place.) Instead of this, we would create rcutils_strcasecmp based on strcasecmp and stricmp? Any thoughts?

Yeah, I have to agree that it would be nicer to have a rcutils_strcasecmp, rather than putting the compatibility code in here. So I'll suggest a new PR to rcutils to do that (sorry that this is going to drag out a bit longer).

For what it is worth, there are 2 different ways we could go about this. One is that we could define rcutils_strcasecmp based on either strcasecmp or stricmp, as you mention above. The other way to do it is to just implement it ourselves, since it is a pretty easy function:

int rcutils_strcasecmp(const char *s1, const char *s2) { const unsigned char *p1 = (const unsigned char *) s1; const unsigned char *p2 = (const unsigned char *) s2; int result; if (p1 == p2) { return 0; } while ((result = tolower(*p1) - tolower(*p2++)) == 0) { if (*p1++ == '\0') { break; } } return result; }

(that code comes from glibc with some minor edits by me). I'm fine with either option, just thought I'd point out that both were available.

@Ada-King

Let's open new PR as discussed here and then get back on this!

One is that we could define rcutils_strcasecmp based on either strcasecmp or stricmp

I am good to with either way, but I personally would prefer this one.

@Ada-King @clalancette

I just created ros2/rcutils#279 and ros2/rcutils#280.

rcl_yaml_param_parser/src/parser.c

iuhilnehc-ynos · 2020-08-24T07:32:38Z

rcl_yaml_param_parser/test/test_parse_yaml.cpp

+  ASSERT_EQ(9U, param_value->double_array_value->size);
+  EXPECT_TRUE(std::isnan(param_value->double_array_value->values[2]));
+  EXPECT_TRUE(std::isnan(param_value->double_array_value->values[3]));
+  EXPECT_TRUE(std::isinf(param_value->double_array_value->values[4]));


@Ada-King

About inf, there are positive infinity and negative infinity.
isinf(x) returns 1 if x is positive infinity, and -1 if x is negative infinity.

Maybe it should be

EXPECT_TRUE(std::isinf(param_value->double_array_value->values[4]) == 1); EXPECT_TRUE(std::isinf(param_value->double_array_value->values[5]) == -1);

Ok, that make sense

The type of return value in c++ is bool, so the current test is sufficient.

Sorry, It's my bad (not to check the c++ version).
I will use c version isinf to check the result accurately, anyway, it's up to you.

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

Ada-King · 2020-08-25T10:48:24Z

@fujitatomoya
Although the PR is not used here, but i think it's still worth doing.

@clalancette friendly ping
I have modified the code based on above comments, Can you help to merge this :) ?

rcl_yaml_param_parser/src/parser.c

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

clalancette · 2020-08-31T18:46:31Z

rcl_yaml_param_parser/src/parser.c

+    if ((0 == strcmp(value, ".nan")) ||
+      (0 == strcmp(value, ".NaN")) ||
+      (0 == strcmp(value, ".NAN")) ||
+      (0 == strcmp(value, ".inf")) ||
+      (0 == strcmp(value, ".Inf")) ||
+      (0 == strcmp(value, ".INF")) ||
+      (0 == strcmp(value, "+.inf")) ||
+      (0 == strcmp(value, "+.Inf")) ||
+      (0 == strcmp(value, "+.INF")) ||
+      (0 == strcmp(value, "-.inf")) ||
+      (0 == strcmp(value, "-.Inf")) ||
+      (0 == strcmp(value, "-.INF")))


Once we get ros2/rcutils#280 in, we can change this code to use rcutils_strcasecmp.

Once we get ros2/rcutils#280 in, we can change this code to use rcutils_strcasecmp.

@clalancette
FYI (#741 (comment))
The yaml spec only allows specific formats.

Besides, the existed build error doesn't seem to be caused by this PR.

I checked it, the build error result from #727, the rcutils dependency has been merged, but build system still based on ros-rolling-rcutils: 1.1.0, which is the problem.

Anyway, this has no effect on this merge process.

clalancette · 2020-09-01T12:45:15Z

@ros-pull-request-builder retest this please

brawner · 2020-09-01T23:15:17Z

The refactoring PR #754 was merged in order to make progress on adding in coverage unit tests. Rebasing this PR on master should be fairly straightforward. The modified logic in parser.c has been moved to parse.c and everything else can remain the same. Here is a draft PR with this PR's commits squashed and rebased into a single commit on top of master. Feel free to cherry-pick that commit, or just use it as an example. This logic also passes all the unit tests I'm adding in #771, #766, and #772.

PR with squashed commits rebased onto master
#781

fujitatomoya · 2020-09-01T23:37:31Z

@brawner

thanks for opening and rebasing the PR, i will check #781 once again just in case.

clalancette · 2020-09-02T12:31:23Z

I'm going to close this one out in favor of #781; we can continue the discussion over there.

Ada-King added 2 commits August 12, 2020 11:27

Fix yaml parser error when meets .nan

2fb4342

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

Correct code style

d12c0f1

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

Ada-King force-pushed the topic-yaml-parser branch from c65d7e1 to d12c0f1 Compare August 12, 2020 03:55

clalancette requested changes Aug 13, 2020

View reviewed changes

rcl_yaml_param_parser/src/parser.c Outdated Show resolved Hide resolved

Modify as suggested

d326731

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

Ada-King force-pushed the topic-yaml-parser branch from d5a457f to d326731 Compare August 18, 2020 02:53

Ada-King closed this Aug 18, 2020

Ada-King reopened this Aug 18, 2020

clalancette requested changes Aug 18, 2020

View reviewed changes

fujitatomoya reviewed Aug 19, 2020

View reviewed changes

rcl_yaml_param_parser/test/special_float2.yaml Outdated Show resolved Hide resolved

Improve test

8d466b2

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

Ada-King requested a review from clalancette August 19, 2020 08:37

clalancette requested changes Aug 19, 2020

View reviewed changes

fujitatomoya approved these changes Aug 20, 2020

View reviewed changes

Ada-King added 2 commits August 20, 2020 10:31

Fix minor flaw

838881e

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

Fix minor flaw again

399c835

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

clalancette approved these changes Aug 20, 2020

View reviewed changes

Satisfy windows CI

56a398b

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

fujitatomoya reviewed Aug 21, 2020

View reviewed changes

fujitatomoya mentioned this pull request Aug 23, 2020

Case insensitive string compare function. ros2/rcutils#279

Closed

iuhilnehc-ynos reviewed Aug 24, 2020

View reviewed changes

rcl_yaml_param_parser/src/parser.c Outdated Show resolved Hide resolved

iuhilnehc-ynos reviewed Aug 24, 2020

View reviewed changes

Change the match rule for special float

1771cd9

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

iuhilnehc-ynos reviewed Aug 25, 2020

View reviewed changes

rcl_yaml_param_parser/src/parser.c Show resolved Hide resolved

Distinguish +.inf and -.inf

5eb83f4

Signed-off-by: Ada-King <Bingtao.Du@sony.com>

fujitatomoya mentioned this pull request Aug 27, 2020

introduce rcutils_strcasecmp, case insensitive string compare. ros2/rcutils#280

Merged

clalancette reviewed Aug 31, 2020

View reviewed changes

clalancette closed this Sep 2, 2020

brawner mentioned this pull request Sep 2, 2020

Fix yaml parser error when meets .nan (refactor on #754) #781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix yaml parser error when meets .nan #741

Fix yaml parser error when meets .nan #741

Ada-King commented Aug 12, 2020

clalancette left a comment

Ada-King commented Aug 18, 2020

Ada-King commented Aug 18, 2020

clalancette left a comment

fujitatomoya left a comment

Ada-King commented Aug 20, 2020

clalancette left a comment

clalancette commented Aug 20, 2020

clalancette commented Aug 20, 2020

fujitatomoya commented Aug 20, 2020

brawner commented Aug 20, 2020

clalancette commented Aug 20, 2020

Ada-King commented Aug 21, 2020

fujitatomoya Aug 21, 2020

clalancette Aug 21, 2020

fujitatomoya Aug 22, 2020

fujitatomoya Aug 23, 2020

iuhilnehc-ynos Aug 24, 2020

Ada-King Aug 24, 2020

Ada-King Aug 25, 2020

iuhilnehc-ynos Aug 25, 2020

Ada-King commented Aug 25, 2020

clalancette Aug 31, 2020

Ada-King Sep 1, 2020

Ada-King Sep 1, 2020

clalancette commented Sep 1, 2020

brawner commented Sep 1, 2020 •

edited

Loading

fujitatomoya commented Sep 1, 2020

clalancette commented Sep 2, 2020

Fix yaml parser error when meets .nan #741

Fix yaml parser error when meets .nan #741

Conversation

Ada-King commented Aug 12, 2020

clalancette left a comment

Choose a reason for hiding this comment

Ada-King commented Aug 18, 2020

Ada-King commented Aug 18, 2020

clalancette left a comment

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

Ada-King commented Aug 20, 2020

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Aug 20, 2020

clalancette commented Aug 20, 2020

fujitatomoya commented Aug 20, 2020

brawner commented Aug 20, 2020

clalancette commented Aug 20, 2020

Ada-King commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ada-King commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Sep 1, 2020

brawner commented Sep 1, 2020 • edited Loading

fujitatomoya commented Sep 1, 2020

clalancette commented Sep 2, 2020

brawner commented Sep 1, 2020 •

edited

Loading