Refactor parser.c for better testability by brawner · Pull Request #754 · ros2/rcl

brawner · 2020-08-19T00:48:08Z

This PR separates out the parser.c functionality into separate source files with their own headers so additional unit tests can be added to target specific functions. I still need to create the additional unit tests, but this current refactor passes the tests that currently exist, which has about 88% coverage.

Because this PR is so large, I've separated the changes into logical commits. Reviewing each commit individually might be the easiest way to scan through this PR as they illustrate the copy-paste changes I did. They could be their own PRs if that's desired by the reviewers.

Main commits

8872020 Shuffles functions into their own separate files, but doesn't change any actual logic
70e0c99 Reorders parser.c to match parser.h
bbb23d2 Creates a couple of macros and reduces some of the duplicate code when copying rcl_yaml_variant_t
5635514 Creates some more macros for deduplicating the add_value_to_array type functions.

Addressing PR Feedback commits

e741afa Removes a debug fprintf
e617716 Various PR feedback
3ff206f Moves impl headers into src directory

brawner · 2020-08-19T00:50:43Z

Testing --packages-select rcl_yaml_param_parser

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2020-08-24T21:01:57Z

@anup-pem do you have time to review this pull request since you originally wrote this? It would help us make sure it doesn’t change the code in a way that you didn’t intend or that might cause apex issues in the future.

hidmic

Overall LGTM, though I didn't try to mix and match new and old lines. I'll try again next week, but I guess our best chance of not introducing regressions lies with tests.

hidmic · 2020-08-28T21:54:23Z

+        RCUTILS_SAFE_FWRITE_TO_STDERR("Error allocating mem\n"); \
+        return RCUTILS_RET_BAD_ALLOC; \
+      } \
+      memmove(val_array->values, tmp_arr, (val_array->size * sizeof(value_type))); \


@brawner perhaps unrelated to this PR, but we can use memcpy instead of memmove. There's no way val_array->values and tmp_arr can overlap.

Switched to memcpy

hidmic · 2020-08-28T22:05:56Z

+
+  rcl_variant_t * param_value = &(params_st->params[node_idx].parameter_values[parameter_idx]);
+
+  // param_value->string_value = rcutils_strdup(value, allocator);


@brawner why keep this?

hidmic · 2020-08-28T22:06:22Z

+      ret = RCUTILS_RET_ERROR;
+      break;
+    case DATA_TYPE_BOOL:
+      if (false == is_seq) {


@brawner probably preexistent, but:

Suggested change

if (false == is_seq) {

if (!is_seq) {

? Same below.

Definitely preexistent. Switched

hidmic · 2020-08-28T22:09:53Z

+            break;
+          }
+
+          memmove(param_name, parameter_ns, params_ns_len);


@brawner perhaps unrelated, we can use memcpy instead.

Switched to memcpy

brawner · 2020-08-29T00:30:06Z

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-08-31T17:57:46Z

This will need to be rebased after #765 and #775 are merged

brawner · 2020-08-31T21:06:26Z

Rebased this onto master after recent PR merges. I updated the commits in the description at the top of this PR if the reviews would rather step through the changes that way. They make it easier to see what changes and what doesn't.

brawner · 2020-08-31T21:09:31Z

Linux
Linux-aarch64
macOS
Windows

hidmic

LGTM, comments are minor or can be addressed in another PR.

hidmic · 2020-09-01T18:31:47Z

+      if (NULL == val_array->values) { \
+        val_array->values = tmp_arr; \
+        RCUTILS_SAFE_FWRITE_TO_STDERR("Error allocating mem\n"); \
+        return RCUTILS_RET_BAD_ALLOC; \


@brawner nit: if ADD_VALUE_TO_SIMPLE_ARRAY took ownership of value and deallocated it in failure cases, add_val_to_* API callers wouldn't have to add a trailing nullity check and deallocation every time one of those functions fails.

The callers already have to handle deallocation of value in several instances. The logic is complicated and this change will lead to larger changes, which I think are out of scope for just adding coverage to this package.

hidmic · 2020-09-01T20:32:13Z

+
+    tot_len = ns_len + sep_len + name_len + 1U;
+
+    cur_ns = allocator.reallocate(cur_ns, tot_len, allocator.state);


@brawner if allocator.reallocate fails, cur_ns will leak.

Moving to #780

hidmic · 2020-09-01T20:33:23Z

+      return RCUTILS_RET_BAD_ALLOC;
+    }
+    memmove((cur_ns + ns_len), sep_str, sep_len);
+    memmove((cur_ns + ns_len + sep_len), name, name_len);


@brawner same about memcpy being cheaper.

hidmic · 2020-09-01T20:34:15Z

+      }
+      if (NULL != last_idx) {
+        tot_len = ((size_t)(last_idx - cur_ns) + 1U);
+        cur_ns = allocator.reallocate(cur_ns, tot_len, allocator.state);


@brawner same about cur_ns leaking on allocator.reallocate failure.

brawner · 2020-09-01T21:53:22Z

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Signed-off-by: Stephen Brawner <brawner@gmail.com> squash! Reorder parser.c to match parser.h

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner · 2020-09-01T22:26:11Z

Rebasing onto master after merging #780. Testing one last time just because it changes a lot of stuff

Linux
Linux-aarch64
macOS
Windows

brawner · 2020-09-01T22:36:09Z

All ci.ros2.org tests pass, and tests pass with rpr job. The failures in the Rpr job are unrelated to this PR and have already been addressed in PRs to rcl I believe.

wjwwood

lgtm (post merge)

* Fix yaml parser error when meets .nan Signed-off-by: Ada-King <Bingtao.Du@sony.com> Correct code style Signed-off-by: Ada-King <Bingtao.Du@sony.com> Modify as suggested Signed-off-by: Ada-King <Bingtao.Du@sony.com> Improve test Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw again Signed-off-by: Ada-King <Bingtao.Du@sony.com> Satisfy windows CI Signed-off-by: Ada-King <Bingtao.Du@sony.com> Change the match rule for special float Signed-off-by: Ada-King <Bingtao.Du@sony.com> Distinguish +.inf and -.inf Signed-off-by: Ada-King <Bingtao.Du@sony.com> * Remove unnecessary #include change. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in two more necessary includes. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Co-authored-by: Ada-King <Bingtao.Du@sony.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>

* Refactor rcl_yaml_param_parser for better testability Signed-off-by: Stephen Brawner <brawner@gmail.com> * Reorder parser.c to match parser.h Signed-off-by: Stephen Brawner <brawner@gmail.com> squash! Reorder parser.c to match parser.h * Refactor yaml_variant.c for deduplication Signed-off-by: Stephen Brawner <brawner@gmail.com> * ADD_VALUE_TO_SIMPLE_ARRAY for deduplication Signed-off-by: Stephen Brawner <brawner@gmail.com> * Remove fprintf Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR Fixup Signed-off-by: Stephen Brawner <brawner@gmail.com> * Move headers to src directory Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR Fixup Signed-off-by: Stephen Brawner <brawner@gmail.com> * Rebase #780 Signed-off-by: Stephen Brawner <brawner@gmail.com>

* Fix yaml parser error when meets .nan Signed-off-by: Ada-King <Bingtao.Du@sony.com> Correct code style Signed-off-by: Ada-King <Bingtao.Du@sony.com> Modify as suggested Signed-off-by: Ada-King <Bingtao.Du@sony.com> Improve test Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw again Signed-off-by: Ada-King <Bingtao.Du@sony.com> Satisfy windows CI Signed-off-by: Ada-King <Bingtao.Du@sony.com> Change the match rule for special float Signed-off-by: Ada-King <Bingtao.Du@sony.com> Distinguish +.inf and -.inf Signed-off-by: Ada-King <Bingtao.Du@sony.com> * Remove unnecessary #include change. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in two more necessary includes. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Co-authored-by: Ada-King <Bingtao.Du@sony.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>

* Refactor rcl_yaml_param_parser for better testability Signed-off-by: Stephen Brawner <brawner@gmail.com> * Reorder parser.c to match parser.h Signed-off-by: Stephen Brawner <brawner@gmail.com> squash! Reorder parser.c to match parser.h * Refactor yaml_variant.c for deduplication Signed-off-by: Stephen Brawner <brawner@gmail.com> * ADD_VALUE_TO_SIMPLE_ARRAY for deduplication Signed-off-by: Stephen Brawner <brawner@gmail.com> * Remove fprintf Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR Fixup Signed-off-by: Stephen Brawner <brawner@gmail.com> * Move headers to src directory Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR Fixup Signed-off-by: Stephen Brawner <brawner@gmail.com> * Rebase #780 Signed-off-by: Stephen Brawner <brawner@gmail.com>

* Refactor parser.c for better testability (#754) * Refactor rcl_yaml_param_parser for better testability Signed-off-by: Stephen Brawner <brawner@gmail.com> * Reorder parser.c to match parser.h Signed-off-by: Stephen Brawner <brawner@gmail.com> squash! Reorder parser.c to match parser.h * Refactor yaml_variant.c for deduplication Signed-off-by: Stephen Brawner <brawner@gmail.com> * ADD_VALUE_TO_SIMPLE_ARRAY for deduplication Signed-off-by: Stephen Brawner <brawner@gmail.com> * Remove fprintf Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR Fixup Signed-off-by: Stephen Brawner <brawner@gmail.com> * Move headers to src directory Signed-off-by: Stephen Brawner <brawner@gmail.com> * PR Fixup Signed-off-by: Stephen Brawner <brawner@gmail.com> * Rebase #780 Signed-off-by: Stephen Brawner <brawner@gmail.com> * Missing includes Signed-off-by: Stephen Brawner <brawner@gmail.com>

* Fix yaml parser error when meets .nan Signed-off-by: Ada-King <Bingtao.Du@sony.com> Correct code style Signed-off-by: Ada-King <Bingtao.Du@sony.com> Modify as suggested Signed-off-by: Ada-King <Bingtao.Du@sony.com> Improve test Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw again Signed-off-by: Ada-King <Bingtao.Du@sony.com> Satisfy windows CI Signed-off-by: Ada-King <Bingtao.Du@sony.com> Change the match rule for special float Signed-off-by: Ada-King <Bingtao.Du@sony.com> Distinguish +.inf and -.inf Signed-off-by: Ada-King <Bingtao.Du@sony.com> * Remove unnecessary #include change. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in two more necessary includes. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Co-authored-by: Ada-King <Bingtao.Du@sony.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>

* Fix yaml parser error when meets .nan Signed-off-by: Ada-King <Bingtao.Du@sony.com> Correct code style Signed-off-by: Ada-King <Bingtao.Du@sony.com> Modify as suggested Signed-off-by: Ada-King <Bingtao.Du@sony.com> Improve test Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw again Signed-off-by: Ada-King <Bingtao.Du@sony.com> Satisfy windows CI Signed-off-by: Ada-King <Bingtao.Du@sony.com> Change the match rule for special float Signed-off-by: Ada-King <Bingtao.Du@sony.com> Distinguish +.inf and -.inf Signed-off-by: Ada-King <Bingtao.Du@sony.com> * Remove unnecessary #include change. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in two more necessary includes. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Co-authored-by: Ada-King <Bingtao.Du@sony.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> Signed-off-by: Stephen Brawner <brawner@gmail.com>

* Fix yaml parser error when meets .nan Signed-off-by: Ada-King <Bingtao.Du@sony.com> Correct code style Signed-off-by: Ada-King <Bingtao.Du@sony.com> Modify as suggested Signed-off-by: Ada-King <Bingtao.Du@sony.com> Improve test Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw Signed-off-by: Ada-King <Bingtao.Du@sony.com> Fix minor flaw again Signed-off-by: Ada-King <Bingtao.Du@sony.com> Satisfy windows CI Signed-off-by: Ada-King <Bingtao.Du@sony.com> Change the match rule for special float Signed-off-by: Ada-King <Bingtao.Du@sony.com> Distinguish +.inf and -.inf Signed-off-by: Ada-King <Bingtao.Du@sony.com> * Remove unnecessary #include change. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in two more necessary includes. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Co-authored-by: Ada-King <Bingtao.Du@sony.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> Signed-off-by: Stephen Brawner <brawner@gmail.com> Co-authored-by: Ada-King <Bingtao.Du@sony.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>

brawner requested review from anup-pem and wjwwood August 19, 2020 00:48

brawner mentioned this pull request Aug 20, 2020

Fix yaml parser error when meets .nan #741

Closed

This was referenced Aug 26, 2020

Set variant values to NULL on fini #765

Merged

Add basic unit tests for refactored functions in rcl_yaml_param_parser (coverage part 1/3) #771

Merged

brawner self-assigned this Aug 27, 2020

brawner requested review from Blast545 and hidmic August 28, 2020 21:11

hidmic reviewed Aug 28, 2020

View reviewed changes

brawner closed this Aug 29, 2020

brawner reopened this Aug 29, 2020

wjwwood reviewed Aug 29, 2020

View reviewed changes

Comment thread rcl_yaml_param_parser/QUALITY_DECLARATION.md Outdated

brawner force-pushed the brawner/rcl_yaml-refactor-srcs branch from 84661d0 to 28a8568 Compare August 29, 2020 00:50

brawner mentioned this pull request Aug 31, 2020

Don't overwrite string data pointer if realloc fails #775

Merged

hidmic reviewed Aug 31, 2020

View reviewed changes

Comment thread rcl_yaml_param_parser/src/parser.c Outdated

brawner force-pushed the brawner/rcl_yaml-refactor-srcs branch from 28a8568 to 3ff206f Compare August 31, 2020 21:01

hidmic approved these changes Sep 1, 2020

View reviewed changes

brawner force-pushed the brawner/rcl_yaml-refactor-srcs branch 2 times, most recently from 9a09bd2 to b062da9 Compare September 1, 2020 21:52

brawner added 4 commits September 1, 2020 15:10

Refactor rcl_yaml_param_parser for better testability

a3718fa

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Reorder parser.c to match parser.h

f22c397

Signed-off-by: Stephen Brawner <brawner@gmail.com> squash! Reorder parser.c to match parser.h

Refactor yaml_variant.c for deduplication

33bcec2

Signed-off-by: Stephen Brawner <brawner@gmail.com>

ADD_VALUE_TO_SIMPLE_ARRAY for deduplication

146c467

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner added 5 commits September 1, 2020 15:10

Remove fprintf

7098edd

Signed-off-by: Stephen Brawner <brawner@gmail.com>

PR Fixup

643beda

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Move headers to src directory

705d230

Signed-off-by: Stephen Brawner <brawner@gmail.com>

PR Fixup

2abe117

Signed-off-by: Stephen Brawner <brawner@gmail.com>

Rebase #780

8d72dc4

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner force-pushed the brawner/rcl_yaml-refactor-srcs branch from b062da9 to 8d72dc4 Compare September 1, 2020 22:11

brawner merged commit 5663f24 into master Sep 1, 2020

delete-merged-branch Bot deleted the brawner/rcl_yaml-refactor-srcs branch September 1, 2020 22:36

wjwwood reviewed Sep 2, 2020

View reviewed changes

brawner mentioned this pull request Sep 2, 2020

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

Merged

brawner mentioned this pull request Sep 2, 2020

Fix rcl_parse_yaml_file() error handling. #776

Merged

brawner mentioned this pull request Sep 2, 2020

[foxy backport] Refactor parser.c for better testability (#754) #784

Merged


		rcl_variant_t * param_value = &(params_st->params[node_idx].parameter_values[parameter_idx]);

		// param_value->string_value = rcutils_strdup(value, allocator);


		tot_len = ns_len + sep_len + name_len + 1U;

		cur_ns = allocator.reallocate(cur_ns, tot_len, allocator.state);

Uh oh!

Conversation

brawner commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brawner commented Aug 19, 2020

Uh oh!

wjwwood commented Aug 24, 2020

Uh oh!

hidmic 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!

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!

brawner commented Aug 29, 2020

Uh oh!

Uh oh!

brawner commented Aug 31, 2020

Uh oh!

Uh oh!

brawner commented Aug 31, 2020

Uh oh!

brawner commented Aug 31, 2020

Uh oh!

hidmic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

brawner commented Sep 1, 2020

Uh oh!

brawner commented Sep 1, 2020

Uh oh!

brawner commented Sep 1, 2020

Uh oh!

wjwwood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brawner commented Aug 19, 2020 •

edited

Loading

hidmic left a comment •

edited

Loading

wjwwood left a comment •

edited

Loading