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

accept z_string_array_t in z_config_client #646

Closed
milyin opened this issue Sep 4, 2024 · 2 comments
Closed

accept z_string_array_t in z_config_client #646

milyin opened this issue Sep 4, 2024 · 2 comments
Assignees
Labels
release Part of the next release

Comments

@milyin
Copy link
Contributor

milyin commented Sep 4, 2024

It was decided to remove z_config_client so not necessary to do this task. z_owned_string_array_t constructors remains implemented in zenoh-c and not implemented yet in pico for now. After adding more convenient config API this string array will be useful, no need to remove it


Update from comments and after discussion: the z_moved_string_array_t* seems to be much more convenient argument for z_config_client, instead of const char *const *peers, size_t n_peers. This update justifies existence of z_owned_string_array_t construction/filling methods both for zenoh-c and zenoh-pico


The z_owned_string_array_t is used in single place: as destination type for z_hello_locators function. It's never accepted by any zenoh function and therefore there is no reason for user to be able to create and modify it.
So functions z_string_array_new, z_string_array_push_by_alias, z_string_array_push_by_copy seems to be not really needed. Zenoh-pico doesn't provide these functions.

The proposal is to make these methods unstable in zenoh-c to not force us to implement them in zenoh-pico.

@DenisBiryukov91
Copy link
Contributor

DenisBiryukov91 commented Sep 4, 2024

we could technically use z_owned_string_array_t instead of *const *const c_char + length in z_config_client when passing array of peers - this would allow accepting non-null-terminated strings (this might also be interesting for c++ to allow accepting a list of string view instead of vector of std::string in the corresponding function - but performance gain would likely be minimal)

@milyin
Copy link
Contributor Author

milyin commented Sep 5, 2024

we could technically use z_owned_string_array_t instead of *const *const c_char + length in z_config_client when passing array of peers - this would allow accepting non-null-terminated strings (this might also be interesting for c++ to allow accepting a list of string view instead of vector of std::string in the corresponding function - but performance gain would likely be minimal)

@sashacmc what do you think? This makes sense for me: the const char *const *peers, size_t n_peers is really ugly, I like the idea to use z_owned_sting_array_t here instead

@milyin milyin changed the title make z_string_array_t methods unstable accept z_string_array_t in z_config_client Sep 6, 2024
@milyin milyin closed this as not planned Won't fix, can't repro, duplicate, stale Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Part of the next release
Projects
Status: Done
Development

No branches or pull requests

2 participants