Support default values for string arrays #197

mikaelarguedas · 2017-01-03T12:19:57Z

related to #206
to be merged along ros2/design#111

CI:
Linux:
OSX:
Windows:

dirk-thomas · 2017-01-03T15:48:43Z

rosidl_generator_c/resource/msg__functions.c.em

+                        lines.append('    goto %s%s;' % (label_prefix, last_label_index))
+                        abort_lines[0:0] = [
+                          '  rosidl_generator_c__String__fini(&msg->%s[%d]);' % (field.name, i),
+                          '%s%d:' % (label_prefix, last_label_index),


Indentation not a multiple of 4.

dirk-thomas · 2017-01-03T15:48:50Z

rosidl_generator_c/resource/msg__functions.c.em

+                    lines.append('    goto %s%s;' % (label_prefix, last_label_index))
+                    abort_lines[0:0] = [
+                      '  rosidl_generator_c__String__fini(&msg->%s.data[%d]);' % (field.name, i),
+                      '%s%d:' % (label_prefix, last_label_index),


Indentation not a multiple of 4.

dirk-thomas · 2017-01-03T15:52:29Z

rosidl_parser/rosidl_parser/__init__.py

@@ -445,8 +445,7 @@ def parse_value_string(type_, value_string):
    if type_.is_primitive_type() and not type_.is_array:
        return parse_primitive_value_string(type_, value_string)

-    # TODO(mikaelarguedas) change this condition once string escape function is implemented
-    if type_.is_primitive_type() and type_.is_array and type_.type != 'string':
+    if type_.is_primitive_type() and type_.is_array:


It looks like this just enables default values for string arrays but without implementing the required string escaping logic? Currently I simply splits on commas - I don't think this is sufficient since the comma can be within a string.

Yes sorry this is still work in progress, the strings are not escaped yet. This is also the case for simple strings. e.g. string def_string 'Hello w"orld!' makes the message generation crash (at least the python generator).

For string arrays it gets even trickier because the design document says that:
string: a string value which can optionally be quoted with either single quotes (') or double quotes (")
We need to make the quoting mandatory to allow differentiation of array elements otherwise it'll be ambiguous

Why does the quoting need to mandatory? It should be possible to parse the following without a problem:

string[] names ['foo', "bar", 'foo"bar', 'bar\'baz', "foo'bar\"baz"]

if we allow to not quote strings it is valid to declare:
string name a, b
this results in a string containing "a, b" and is totally valid.

Using the same logic for arrays it's valid to declare:
string[] names [a, b, c, d]
in that case we don't know if it's a single element containing `"a, b, c, d" or four element separated by commas or any other combination.
That's what I meant by making the quoting mandatory. Otherwise we'd need to define a delimiter that is not allowed in strings which doesn't make much sense.

Actually we do know what the default values should be in that case. Since the individual items are not quoted the default value contains an array with four elements. Otherwise they would need to be quoted.

But this is only for the message definition files right? I don't see what the command line has to do with this.

We might want to write only one parser and not multiple for different use cases?

I guess, but I still feel that keeping the idl format as simple as possible will be best because we'll likely want to parse it in other languages at some point.

It's the difference in strings for JSON and strings for YAML. YAML is nice, but it's also complicated.

I don't think allowing single quotes is unnecessarily burdensome, so I'm fine with that, I was just trying to find ways to keep is simple. But not requiring the quotes seems unnecessary to me. For ROS 1, we use yaml syntax on the command line (which worked really well and allowed us to only specify parts of the message). So, if we use that again, I don't think we need a super flexible parser that can work here and on any arbitrary input from the CLI.

I agree with @wjwwood that it would be simpler and clearer if we require users to quote and escape the interior quotes. Not only to reduce parser complexity but also improve human readability.

I agree that it all comes down to how (what format) we expect people to pass parameters (both CLI and IDL/config files). Do we have a document or discussion somewhere so that we could iterate on and refer to before making a decision on the scope of this parser?

mikaelarguedas · 2017-02-07T23:54:20Z

This should be updated according to ros2/design#111

mikaelarguedas · 2017-04-05T18:36:29Z

@mikaelarguedas look at this someday !

…ke escaped IDL strings are broken in C++)

mikaelarguedas · 2018-06-13T12:20:58Z

Linux
Linux-aarch64
macOS
Windows

CI testing only rosidl_parser with the linter fixup:

Linux
Windows

dirk-thomas · 2018-06-13T16:35:33Z

rosidl_generator_c/test/test_interfaces.c

@@ -295,6 +310,10 @@ int test_strings(void)
  EXPECT_EQ(true, res);
  EXPECT_EQ(0, strcmp(strings->empty_string.data, TEST_STRING));
  EXPECT_EQ(0, strcmp(strings->def_string.data, "Hello world!"));
+  EXPECT_EQ(0, strcmp(strings->def_string2.data, "Hello'world!"));
+  EXPECT_EQ(0, strcmp(strings->def_string3.data, "Hello\"world!"));
+  EXPECT_EQ(0, strcmp(strings->def_string4.data, "Hello\'world!"));


Based on the message definition:

string def_string4 'Hello'world!'

Why is there a backslash in the C++ string literal? Should it be just "Hello'world!"?

Same for def_various_quotes.data[1].data.

hmmm good question. I guess as the escaping is not necessary in C because we use double quotes it should get "unescaped" by the C generator when it gets it from the parser

The extra backslash should be removed then.

Done in efe98ea

dirk-thomas

lgtm

mikaelarguedas added the in progress Actively being worked on (Kanban column) label Jan 3, 2017

mikaelarguedas self-assigned this Jan 3, 2017

dirk-thomas reviewed Jan 3, 2017

View reviewed changes

mikaelarguedas changed the title ~~Support default values for string arrays~~ Support default values for string arrays Jan 9, 2017

mikaelarguedas added ready Work is about to start (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 9, 2017

mikaelarguedas mentioned this pull request Feb 2, 2017

Fix string delimiter #204

Merged

This was referenced Feb 8, 2017

Default values for all defined types #206

Open

[rosidl_parser] Parse strings default values in .msg files #170

Closed

mikaelarguedas force-pushed the string_arrays_default branch from 9afbd64 to 04d0532 Compare July 5, 2017 02:14

mikaelarguedas mentioned this pull request Oct 6, 2017

Zero initialization #236

Merged

mikaelarguedas force-pushed the string_arrays_default branch 2 times, most recently from 236d717 to c297959 Compare December 10, 2017 04:56

mikaelarguedas mentioned this pull request Dec 17, 2017

add rules for array default values ros2/design#111

Merged

clalancette added this to the bouncy milestone Feb 22, 2018

mikaelarguedas and others added 13 commits June 13, 2018 10:55

[rosidl_generator_c] test string array default values

83de658

[rosidl_parser] allow parsing of string array default values

52dfc5a

[rosidl_generator_cpp] test string arrays default values

185e5bc

[rosidl_generator_py] test string array default values

bc0ad63

test all uppercase letters too

f2215ab

reappply 5491379

9202e57

[rosidl_generator_c] fix message include in test

cdd63b4

add TODO for string array default value parsing

6b85824

[rosidl_generator_c] use primitive_to_c to get escaped strings

9f83ac0

[rosidl_parser] parse string array default values

1c400e0

remove debug prints

62dfe33

[rosidl_parser] add tests for string parsing

1fcd25d

[rosidl_parser] corner cases

1206f44

mikaelarguedas added 10 commits June 13, 2018 10:55

test string and string array default values

e654b83

[rosidl_generator_cpp] test string array default values (wip looks li…

4705846

…ke escaped IDL strings are broken in C++)

[rosidl_generator_py] homogenize messages and test string array defaults

f273c68

throw on leading commas or consecutive commas in array definition

287cea6

update tests accordingly

469f694

fix indent in empy template

1f9b604

striping whitespace

a4bdebc

remove debug print

70b44ab

fixup error message

eb12fa2

[rosidl_generator_cpp] support default values for string arrays

9f2f54c

mikaelarguedas force-pushed the string_arrays_default branch from 43ff7b9 to 9f2f54c Compare June 13, 2018 09:27

mikaelarguedas added 2 commits June 13, 2018 12:14

shortest english pangram

b95a053

quote fixup

966df1c

mikaelarguedas added in review Waiting for review (Kanban column) and removed ready Work is about to start (Kanban column) labels Jun 13, 2018

dirk-thomas reviewed Jun 13, 2018

View reviewed changes

remove unnecessary backslash

efe98ea

dirk-thomas approved these changes Jun 13, 2018

View reviewed changes

mikaelarguedas merged commit 6549399 into master Jun 14, 2018

mikaelarguedas deleted the string_arrays_default branch June 14, 2018 04:27

wjwwood removed the in review Waiting for review (Kanban column) label Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support default values for string arrays #197

Support default values for string arrays #197

mikaelarguedas commented Jan 3, 2017 •

edited

Loading

dirk-thomas Jan 3, 2017

dirk-thomas Jan 3, 2017

dirk-thomas Jan 3, 2017

mikaelarguedas Jan 3, 2017

dirk-thomas Jan 3, 2017 •

edited

Loading

mikaelarguedas Jan 3, 2017

dirk-thomas Jan 3, 2017

wjwwood Jan 3, 2017

dirk-thomas Jan 3, 2017

wjwwood Jan 3, 2017

wjwwood Jan 3, 2017

mikaelarguedas Jan 4, 2017

mikaelarguedas commented Feb 7, 2017

mikaelarguedas commented Apr 5, 2017

mikaelarguedas commented Jun 13, 2018

dirk-thomas Jun 13, 2018

mikaelarguedas Jun 13, 2018

dirk-thomas Jun 13, 2018

mikaelarguedas Jun 13, 2018

dirk-thomas left a comment

Support default values for string arrays #197

Support default values for string arrays #197

Conversation

mikaelarguedas commented Jan 3, 2017 • edited Loading

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

dirk-thomas Jan 3, 2017 • edited Loading

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

mikaelarguedas commented Feb 7, 2017

mikaelarguedas commented Apr 5, 2017

mikaelarguedas commented Jun 13, 2018

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

dirk-thomas left a comment

Choose a reason for hiding this comment

mikaelarguedas commented Jan 3, 2017 •

edited

Loading

dirk-thomas Jan 3, 2017 •

edited

Loading