From 2f25cb132cfec7f50f59bf54189b08c26af9d6fa Mon Sep 17 00:00:00 2001 From: Brian Date: Tue, 7 Dec 2021 14:02:05 +0000 Subject: [PATCH] address feedback, remove descriptor name, change field naming --- .../include/rcl_yaml_param_parser/parser.h | 2 +- .../include/rcl_yaml_param_parser/types.h | 3 +- .../src/node_params_descriptors.c | 16 ++++----- rcl_yaml_param_parser/src/parse.c | 36 +++++++++++++------ rcl_yaml_param_parser/src/parser.c | 10 ++---- rcl_yaml_param_parser/src/yaml_descriptor.c | 13 +------ .../test/test_node_params_descriptors.cpp | 14 ++++---- .../test/test_parse_yaml.cpp | 8 ----- .../test/test_yaml_descriptor.cpp | 7 ---- 9 files changed, 46 insertions(+), 63 deletions(-) diff --git a/rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h b/rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h index 03cd7c17cf..131a92365d 100644 --- a/rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h +++ b/rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h @@ -121,7 +121,7 @@ rcl_variant_t * rcl_yaml_node_struct_get( /// \brief Get the descriptor struct for a given parameter /// \param[in] node_name is the name of the node to which the parameter belongs -/// \param[in] param_name is the name of the parameter whose value is to be retrieved +/// \param[in] param_name is the name of the parameter whose descriptor is to be retrieved /// \param[inout] params_st points to the populated (or to be populated) parameter struct /// \return parameter descriptor struct on success and NULL on failure RCL_YAML_PARAM_PARSER_PUBLIC diff --git a/rcl_yaml_param_parser/include/rcl_yaml_param_parser/types.h b/rcl_yaml_param_parser/include/rcl_yaml_param_parser/types.h index dd8c1142af..dc53184dd3 100644 --- a/rcl_yaml_param_parser/include/rcl_yaml_param_parser/types.h +++ b/rcl_yaml_param_parser/include/rcl_yaml_param_parser/types.h @@ -99,7 +99,6 @@ typedef struct rcl_node_params_s /// \brief param_descriptor_t stores the descriptor of a parameter typedef struct rcl_param_descriptor_s { - char * name; bool * read_only; uint8_t * type; char * description; @@ -118,7 +117,7 @@ typedef struct rcl_node_params_descriptors_s { char ** parameter_names; ///< Array of parameter names (keys) rcl_param_descriptor_t * parameter_descriptors; ///< Array of corresponding parameter descriptors - size_t num_params; ///< Number of parameters in the node + size_t num_descriptors; ///< Number of parameters in the node size_t capacity_descriptors; ///< Capacity of parameters in the node } rcl_node_params_descriptors_t; diff --git a/rcl_yaml_param_parser/src/node_params_descriptors.c b/rcl_yaml_param_parser/src/node_params_descriptors.c index 6ad3539419..5eb0032523 100644 --- a/rcl_yaml_param_parser/src/node_params_descriptors.c +++ b/rcl_yaml_param_parser/src/node_params_descriptors.c @@ -51,11 +51,11 @@ rcutils_ret_t node_params_descriptors_init_with_capacity( if (NULL == node_descriptors->parameter_descriptors) { allocator.deallocate(node_descriptors->parameter_names, allocator.state); node_descriptors->parameter_names = NULL; - RCUTILS_SET_ERROR_MSG("Failed to allocate memory for node parameter values"); + RCUTILS_SET_ERROR_MSG("Failed to allocate memory for node parameter descriptors"); return RCUTILS_RET_BAD_ALLOC; } - node_descriptors->num_params = 0U; + node_descriptors->num_descriptors = 0U; node_descriptors->capacity_descriptors = capacity; return RCUTILS_RET_OK; } @@ -68,12 +68,12 @@ rcutils_ret_t node_params_descriptors_reallocate( RCUTILS_CHECK_ARGUMENT_FOR_NULL(node_descriptors, RCUTILS_RET_INVALID_ARGUMENT); RCUTILS_CHECK_ALLOCATOR_WITH_MSG( &allocator, "invalid allocator", return RCUTILS_RET_INVALID_ARGUMENT); - // invalid if new_capacity is less than num_params - if (new_capacity < node_descriptors->num_params) { + // invalid if new_capacity is less than num_descriptors + if (new_capacity < node_descriptors->num_descriptors) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( "new capacity '%zu' must be greater than or equal to '%zu'", new_capacity, - node_descriptors->num_params); + node_descriptors->num_descriptors); return RCUTILS_RET_INVALID_ARGUMENT; } @@ -119,7 +119,7 @@ void rcl_yaml_node_params_descriptors_fini( } if (NULL != node_descriptors->parameter_names) { - for (size_t parameter_idx = 0U; parameter_idx < node_descriptors->num_params; + for (size_t parameter_idx = 0U; parameter_idx < node_descriptors->num_descriptors; parameter_idx++) { char * param_name = node_descriptors->parameter_names[parameter_idx]; @@ -132,7 +132,7 @@ void rcl_yaml_node_params_descriptors_fini( } if (NULL != node_descriptors->parameter_descriptors) { - for (size_t parameter_idx = 0U; parameter_idx < node_descriptors->num_params; + for (size_t parameter_idx = 0U; parameter_idx < node_descriptors->num_descriptors; parameter_idx++) { rcl_yaml_descriptor_fini( @@ -143,6 +143,6 @@ void rcl_yaml_node_params_descriptors_fini( node_descriptors->parameter_descriptors = NULL; } - node_descriptors->num_params = 0; + node_descriptors->num_descriptors = 0; node_descriptors->capacity_descriptors = 0; } diff --git a/rcl_yaml_param_parser/src/parse.c b/rcl_yaml_param_parser/src/parse.c index f1814cda33..ffa46640ef 100644 --- a/rcl_yaml_param_parser/src/parse.c +++ b/rcl_yaml_param_parser/src/parse.c @@ -495,6 +495,13 @@ rcutils_ret_t parse_descriptor( rcl_param_descriptor_t * param_descriptor = &(params_st->descriptors[node_idx].parameter_descriptors[parameter_idx]); + rcl_node_params_descriptors_t * node_descriptors_st = &(params_st->descriptors[node_idx]); + + // Increase descriptor count only once when a new parameter descriptor value is parsed + if (parameter_idx == node_descriptors_st->num_descriptors) { + node_descriptors_st->num_descriptors++; + } + data_types_t val_type; void * ret_val = get_value(value, style, &val_type, allocator); if (NULL == ret_val) { @@ -508,6 +515,7 @@ rcutils_ret_t parse_descriptor( "Parameter assignment at line %d unallowed in " PARAMS_DESCRIPTORS_KEY, line_num); return RCUTILS_RET_ERROR; } + // If parsing a yaml value, then current parameter namespace must be parameter name allocator.deallocate( params_st->descriptors[node_idx].parameter_names[parameter_idx], @@ -515,13 +523,6 @@ rcutils_ret_t parse_descriptor( params_st->descriptors[node_idx].parameter_names[parameter_idx] = rcutils_strdup(ns_tracker->parameter_ns, allocator); - if (NULL == param_descriptor->name) { - param_descriptor->name = - rcutils_strdup(params_st->descriptors[node_idx].parameter_names[parameter_idx], allocator); - rcl_node_params_descriptors_t * node_descriptors_st = &(params_st->descriptors[node_idx]); - node_descriptors_st->num_params++; - } - if (NULL == ns_tracker->descriptor_key_ns) { RCUTILS_SET_ERROR_MSG("Internal error: Invalid mem"); return RCUTILS_RET_ERROR; @@ -1312,19 +1313,32 @@ rcutils_ret_t find_descriptor( assert(node_idx < param_st->num_nodes); rcl_node_params_descriptors_t * node_descriptors_st = &(param_st->descriptors[node_idx]); - for (*parameter_idx = 0U; *parameter_idx < node_descriptors_st->num_params; (*parameter_idx)++) { + for (*parameter_idx = 0U; *parameter_idx < node_descriptors_st->num_descriptors; + (*parameter_idx)++) + { if (0 == strcmp(node_descriptors_st->parameter_names[*parameter_idx], parameter_name)) { - // Parameter found. + // Descriptor found. return RCUTILS_RET_OK; } } - // Parameter not found, add it. + // Descriptor not found, add it. rcutils_allocator_t allocator = param_st->allocator; + // Reallocate if necessary + if (node_descriptors_st->num_descriptors >= node_descriptors_st->capacity_descriptors) { + if (RCUTILS_RET_OK != node_params_descriptors_reallocate( + node_descriptors_st, node_descriptors_st->capacity_descriptors * 2, allocator)) + { + return RCUTILS_RET_BAD_ALLOC; + } + } + if (NULL != node_descriptors_st->parameter_names[*parameter_idx]) { + param_st->allocator.deallocate( + node_descriptors_st->parameter_names[*parameter_idx], param_st->allocator.state); + } node_descriptors_st->parameter_names[*parameter_idx] = rcutils_strdup(parameter_name, allocator); if (NULL == node_descriptors_st->parameter_names[*parameter_idx]) { return RCUTILS_RET_BAD_ALLOC; } - // node_descriptors_st->num_params++; return RCUTILS_RET_OK; } diff --git a/rcl_yaml_param_parser/src/parser.c b/rcl_yaml_param_parser/src/parser.c index 9bef720b93..6efd6a2ae3 100644 --- a/rcl_yaml_param_parser/src/parser.c +++ b/rcl_yaml_param_parser/src/parser.c @@ -227,14 +227,14 @@ rcl_params_t * rcl_yaml_node_struct_copy( } goto fail; } - for (size_t desc_idx = 0U; desc_idx < node_params_descriptors_st->num_params; ++desc_idx) { + for (size_t desc_idx = 0U; desc_idx < node_params_descriptors_st->num_descriptors; ++desc_idx) { out_node_params_descriptors_st->parameter_names[desc_idx] = rcutils_strdup(node_params_descriptors_st->parameter_names[desc_idx], allocator); if (NULL == out_node_params_descriptors_st->parameter_names[desc_idx]) { RCUTILS_SAFE_FWRITE_TO_STDERR("Error allocating mem\n"); goto fail; } - out_node_params_descriptors_st->num_params++; + out_node_params_descriptors_st->num_descriptors++; const rcl_param_descriptor_t * param_descriptor = &(node_params_descriptors_st->parameter_descriptors[desc_idx]); @@ -542,7 +542,7 @@ void rcl_yaml_node_struct_print( if (NULL != params_st->descriptors) { rcl_node_params_descriptors_t * node_descriptors_st = &(params_st->descriptors[node_idx]); - for (size_t parameter_idx = 0U; parameter_idx < node_descriptors_st->num_params; + for (size_t parameter_idx = 0U; parameter_idx < node_descriptors_st->num_descriptors; parameter_idx++) { if (NULL != node_descriptors_st->parameter_names) { @@ -552,10 +552,6 @@ void rcl_yaml_node_struct_print( } rcl_param_descriptor_t * descriptor = &(node_descriptors_st->parameter_descriptors[parameter_idx]); - if (NULL != descriptor->name) { - printf("\n%*sname: ", param_col + 2, ""); - printf("%s", descriptor->name); - } if (NULL != descriptor->description) { printf("\n%*sdescription: ", param_col + 2, ""); printf("%s", descriptor->description); diff --git a/rcl_yaml_param_parser/src/yaml_descriptor.c b/rcl_yaml_param_parser/src/yaml_descriptor.c index 4424fd6273..5077309321 100644 --- a/rcl_yaml_param_parser/src/yaml_descriptor.c +++ b/rcl_yaml_param_parser/src/yaml_descriptor.c @@ -28,10 +28,6 @@ void rcl_yaml_descriptor_fini( return; } - if (NULL != param_descriptor->name) { - allocator.deallocate(param_descriptor->name, allocator.state); - param_descriptor->name = NULL; - } if (NULL != param_descriptor->read_only) { allocator.deallocate(param_descriptor->read_only, allocator.state); param_descriptor->read_only = NULL; @@ -82,14 +78,7 @@ bool rcl_yaml_descriptor_copy( if (NULL == param_descriptor || NULL == out_param_descriptor) { return false; } - if (NULL != param_descriptor->name) { - out_param_descriptor->name = - rcutils_strdup(param_descriptor->name, allocator); - if (NULL == out_param_descriptor->name) { - RCUTILS_SAFE_FWRITE_TO_STDERR("Error allocating mem"); - return false; - } - } + if (NULL != param_descriptor->description) { out_param_descriptor->description = rcutils_strdup(param_descriptor->description, allocator); diff --git a/rcl_yaml_param_parser/test/test_node_params_descriptors.cpp b/rcl_yaml_param_parser/test/test_node_params_descriptors.cpp index a9d435b68f..1d9a20d022 100644 --- a/rcl_yaml_param_parser/test/test_node_params_descriptors.cpp +++ b/rcl_yaml_param_parser/test/test_node_params_descriptors.cpp @@ -23,12 +23,12 @@ TEST(TestNodeParamsDescriptors, init_fini) { EXPECT_EQ(RCUTILS_RET_OK, node_params_descriptors_init(&node_descriptors, allocator)); EXPECT_NE(nullptr, node_descriptors.parameter_names); EXPECT_NE(nullptr, node_descriptors.parameter_descriptors); - EXPECT_EQ(0u, node_descriptors.num_params); + EXPECT_EQ(0u, node_descriptors.num_descriptors); EXPECT_EQ(128u, node_descriptors.capacity_descriptors); rcl_yaml_node_params_descriptors_fini(&node_descriptors, allocator); EXPECT_EQ(nullptr, node_descriptors.parameter_names); EXPECT_EQ(nullptr, node_descriptors.parameter_descriptors); - EXPECT_EQ(0u, node_descriptors.num_params); + EXPECT_EQ(0u, node_descriptors.num_descriptors); EXPECT_EQ(0u, node_descriptors.capacity_descriptors); // This function doesn't return anything, so just check it doesn't segfault on the second try @@ -44,12 +44,12 @@ TEST(TestNodeParamsDescriptors, init_with_capacity_fini) { &node_descriptors, 1024, allocator)); EXPECT_NE(nullptr, node_descriptors.parameter_names); EXPECT_NE(nullptr, node_descriptors.parameter_descriptors); - EXPECT_EQ(0u, node_descriptors.num_params); + EXPECT_EQ(0u, node_descriptors.num_descriptors); EXPECT_EQ(1024u, node_descriptors.capacity_descriptors); rcl_yaml_node_params_descriptors_fini(&node_descriptors, allocator); EXPECT_EQ(nullptr, node_descriptors.parameter_names); EXPECT_EQ(nullptr, node_descriptors.parameter_descriptors); - EXPECT_EQ(0u, node_descriptors.num_params); + EXPECT_EQ(0u, node_descriptors.num_descriptors); EXPECT_EQ(0u, node_descriptors.capacity_descriptors); // This function doesn't return anything, so just check it doesn't segfault on the second try @@ -65,17 +65,17 @@ TEST(TestNodeParamsDescriptors, reallocate_with_capacity_fini) { &node_descriptors, 1024, allocator)); EXPECT_NE(nullptr, node_descriptors.parameter_names); EXPECT_NE(nullptr, node_descriptors.parameter_descriptors); - EXPECT_EQ(0u, node_descriptors.num_params); + EXPECT_EQ(0u, node_descriptors.num_descriptors); EXPECT_EQ(1024u, node_descriptors.capacity_descriptors); EXPECT_EQ(RCUTILS_RET_OK, node_params_descriptors_reallocate(&node_descriptors, 2048, allocator)); EXPECT_NE(nullptr, node_descriptors.parameter_names); EXPECT_NE(nullptr, node_descriptors.parameter_descriptors); - EXPECT_EQ(0u, node_descriptors.num_params); + EXPECT_EQ(0u, node_descriptors.num_descriptors); EXPECT_EQ(2048u, node_descriptors.capacity_descriptors); rcl_yaml_node_params_descriptors_fini(&node_descriptors, allocator); EXPECT_EQ(nullptr, node_descriptors.parameter_names); EXPECT_EQ(nullptr, node_descriptors.parameter_descriptors); - EXPECT_EQ(0u, node_descriptors.num_params); + EXPECT_EQ(0u, node_descriptors.num_descriptors); EXPECT_EQ(0u, node_descriptors.capacity_descriptors); // This function doesn't return anything, so just check it doesn't segfault on the second try diff --git a/rcl_yaml_param_parser/test/test_parse_yaml.cpp b/rcl_yaml_param_parser/test/test_parse_yaml.cpp index f60c31486a..0cef8ecb0d 100644 --- a/rcl_yaml_param_parser/test/test_parse_yaml.cpp +++ b/rcl_yaml_param_parser/test/test_parse_yaml.cpp @@ -709,8 +709,6 @@ TEST(test_file_parser, correct_syntax_descriptors) { rcl_param_descriptor_t * param_descriptor = NULL; param_descriptor = rcl_yaml_node_struct_get_descriptor("node_ns/node1", "param1", params); ASSERT_TRUE(NULL != param_descriptor); - ASSERT_TRUE(NULL != param_descriptor->name); - EXPECT_STREQ("param1", param_descriptor->name); ASSERT_TRUE(NULL != param_descriptor->description); EXPECT_STREQ("int parameter", param_descriptor->description); ASSERT_TRUE(NULL != param_descriptor->additional_constraints); @@ -728,8 +726,6 @@ TEST(test_file_parser, correct_syntax_descriptors) { param_descriptor = rcl_yaml_node_struct_get_descriptor("node_ns/node1", "param2", params); ASSERT_TRUE(NULL != param_descriptor); - ASSERT_TRUE(NULL != param_descriptor->name); - EXPECT_STREQ("param2", param_descriptor->name); ASSERT_TRUE(NULL != param_descriptor->description); EXPECT_STREQ("double parameter", param_descriptor->description); ASSERT_TRUE(NULL != param_descriptor->min_value_double); @@ -739,8 +735,6 @@ TEST(test_file_parser, correct_syntax_descriptors) { param_descriptor = rcl_yaml_node_struct_get_descriptor("node_ns/node2", "foo.bar", params); ASSERT_TRUE(NULL != param_descriptor); - ASSERT_TRUE(NULL != param_descriptor->name); - EXPECT_STREQ("foo.bar", param_descriptor->name); ASSERT_TRUE(NULL != param_descriptor->description); EXPECT_STREQ("namespaced parameter", param_descriptor->description); ASSERT_TRUE(NULL != param_descriptor->read_only); @@ -748,8 +742,6 @@ TEST(test_file_parser, correct_syntax_descriptors) { param_descriptor = rcl_yaml_node_struct_get_descriptor("node_ns/node2", "foo.baz", params); ASSERT_TRUE(NULL != param_descriptor); - ASSERT_TRUE(NULL != param_descriptor->name); - EXPECT_STREQ("foo.baz", param_descriptor->name); ASSERT_TRUE(NULL != param_descriptor->description); EXPECT_STREQ("other namespaced parameter", param_descriptor->description); diff --git a/rcl_yaml_param_parser/test/test_yaml_descriptor.cpp b/rcl_yaml_param_parser/test/test_yaml_descriptor.cpp index f7350c8a89..2b35f006d7 100644 --- a/rcl_yaml_param_parser/test/test_yaml_descriptor.cpp +++ b/rcl_yaml_param_parser/test/test_yaml_descriptor.cpp @@ -84,9 +84,6 @@ TEST(TestYamlDescriptor, copy_string_fields) { rcl_param_descriptor_t dest_descriptor{}; rcutils_allocator_t allocator = rcutils_get_default_allocator(); - char * tmp_name = rcutils_strdup("param_name", allocator); - ASSERT_STREQ("param_name", tmp_name); - char * tmp_description = rcutils_strdup("param description", allocator); ASSERT_STREQ("param description", tmp_description); @@ -95,24 +92,20 @@ TEST(TestYamlDescriptor, copy_string_fields) { OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { - allocator.deallocate(tmp_name, allocator.state); allocator.deallocate(tmp_description, allocator.state); allocator.deallocate(tmp_additional_constraints, allocator.state); }); - src_descriptor.name = tmp_name; src_descriptor.description = tmp_description; src_descriptor.additional_constraints = tmp_additional_constraints; EXPECT_TRUE(rcl_yaml_descriptor_copy(&dest_descriptor, &src_descriptor, allocator)); - ASSERT_NE(nullptr, dest_descriptor.name); ASSERT_NE(nullptr, dest_descriptor.description); ASSERT_NE(nullptr, dest_descriptor.additional_constraints); OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( { rcl_yaml_descriptor_fini(&dest_descriptor, allocator); }); - EXPECT_STREQ(tmp_name, dest_descriptor.name); EXPECT_STREQ(tmp_description, dest_descriptor.description); EXPECT_STREQ(tmp_additional_constraints, dest_descriptor.additional_constraints); }