Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dynsub: fix bounded strings #1812

Merged

Conversation

eboasson
Copy link
Contributor

No description provided.

Signed-off-by: Erik Boasson <[email protected]>
Copy link
Contributor

@noxpardalis noxpardalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I had one point of clarification I asked in the comments as I'm a little unfamiliar with the code.

Comment on lines +90 to +92
printf ("\"%s\"", p);
else
printf ("\"%s\"", *((const char **) p));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to clarify my comprehension that this is involving some sort of small string/sequence optimization where the characters are immediately available at p for small strings but requires pointer indirection otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone way back when (not me, I just inherited it) chose to map bounded strings to inlined arrays of characters, even though the IDL-to-C mapping specifies it is a pointer just like an unbounded string.

There is a reasonable argument: this is one of very few decent ways to support bounded strings without requiring memory allocation on reading samples. You can do preallocation of memory for sequences, structs, arrays, unions, ... anything, but not for strings in the official mapping. So it makes sense.

With XTypes, you can declare it @external and you'll get a pointer anyway if you really want. (dynsub isn't supporting that (yet).)

@eboasson eboasson merged commit b6fe21d into eclipse-cyclonedds:master Aug 28, 2023
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants