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

correct some API functions #647

Closed
milyin opened this issue Sep 4, 2024 · 1 comment
Closed

correct some API functions #647

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

Comments

@milyin
Copy link
Contributor

milyin commented Sep 4, 2024

This functions should be corrected to use move semantic:

pub extern "C" fn zc_init_logging_with_callback(
    min_severity: zc_log_severity_t,
    callback: &mut zc_owned_closure_log_t,
) {

This should return error value, as z_check is internal now:

/// Tries to construct ZShmMut slice from ZShm slice
#[no_mangle]
pub extern "C" fn z_shm_mut_try_from_immut(
    this: &mut MaybeUninit<z_owned_shm_mut_t>,
    that: &mut z_moved_shm_t,
) {
    let shm: Option<ZShmMut> = that
        .take_rust_type()
        .take()
        .and_then(|val| val.try_into().ok());
    this.as_rust_type_mut_uninit().write(shm);
}

Is the order of parameters of this function right? It seems that list should be first

pub extern "C" fn zc_shm_client_list_add_client(
    id: z_protocol_id_t,
    client: &mut z_moved_shm_client_t,
    list: &mut zc_loaned_shm_client_list_t,
) -> z_result_t {

The logic of placing destination parameter in zenoh-c is this:

  • if function is constructor of something, the destination is on the first place
  • if function is a getter, which constructs something, the destination is after the source
    Seems that this should be second case: it looks more like getter, so this should be the first
pub extern "C" fn z_memory_layout_get_data(
    out_size: &mut MaybeUninit<usize>,
    out_alignment: &mut MaybeUninit<z_alloc_alignment_t>,
    this: &z_loaned_memory_layout_t,
) {
@milyin
Copy link
Contributor Author

milyin commented Sep 13, 2024

last issue fixed in
#684

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