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

zlistx comparators should return -1, 0, or 1 #6398

Open
chu11 opened this issue Oct 30, 2024 · 2 comments
Open

zlistx comparators should return -1, 0, or 1 #6398

chu11 opened this issue Oct 30, 2024 · 2 comments

Comments

@chu11
Copy link
Member

chu11 commented Oct 30, 2024

Per comment in #6370

The zlistx comments say

// Set a user-defined comparator for zlistx_find and zlistx_sort; the method
// must return -1, 0, or 1 depending on whether item1 is less than, equal to,
// or greater than, item2.

We actually don't do this in a nice chunk of flux, we return < 0, 0, or > 0. Examples

static int by_rank (const void *item1, const void *item2)
{
    const struct rnode *x = item1;
    const struct rnode *y = item2;
    return (x->rank - y->rank);
}
static int plugin_byname (const void *item1, const void *item2)
{
    const char *name1 = jobtap_plugin_name ((flux_plugin_t *) item1);
    const char *name2 = item2;
    if (!name1 || !name2)
        return -1;
    return strcmp (name1, name2);
}

My mini audit founds cases in rlist, taskmap, and job-manager.

This doesn't seem to affect zlistx at the moment, but should probably clean this up to adhere to requested API.

@grondo
Copy link
Contributor

grondo commented Nov 13, 2024

The zlistx unit tests use a comparator of strcmp(3):

// Use item handlers
zlistx_set_destructor (list, (zlistx_destructor_fn *) zstr_free);
zlistx_set_duplicator (list, (zlistx_duplicator_fn *) strdup);
zlistx_set_comparator (list, (zlistx_comparator_fn *) strcmp);

Is this really an issue?

@chu11
Copy link
Member Author

chu11 commented Nov 13, 2024

I suppose this is just a anal retentive thing, we're technically not doing what the documentation says. So they could presumably change things in the future (I know super low odds). Skimming zlistx.c, it seems that == 1 or == -1 doesn't matter as everything is > or <=.

Perhaps the more correct thing is to try and update zeromq documentation upstream.

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

No branches or pull requests

2 participants