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

Type of internal struct member causes lots of sign conversion warnings #2780

Open
ZedThree opened this issue Oct 25, 2023 · 4 comments
Open

Comments

@ZedThree
Copy link
Contributor

In current main, there is a struct which is used as a header for many other objects:

typedef struct NC_OBJ
{
    NC_SORT sort; /**< Type of object. */
    char* name;   /**< Name, assumed to be null terminated. */
    size_t id;    /**< This objects ID. */
} NC_OBJ;

Here, id is a size_t, but in many places it's used or passed to an int. In fact, a quick grep shows it's used in ~240 places and it generates ~120 warnings. Switching it to an int only generates 5 warnings.

I think it's mostly used to store and look up various object IDs which are almost always int (at least in the public API), so perhaps that makes more sense?

@DennisHeimbigner
Copy link
Collaborator

I think this is a reasonable change. I will put up a PR for it.

DennisHeimbigner added a commit to DennisHeimbigner/netcdf-c that referenced this issue Oct 25, 2023
re: Unidata#2780

As noted in the above issue, changing the NC_OBJ.id field
type from size_t to int reduces irrelevant warning.
There is no semantic effect since the number of distinct ids
will never approach the max positive integer value.
Note that this could change in the future if the id becomes
more than a simple counter.
@DennisHeimbigner
Copy link
Collaborator

See PR #2781

@ZedThree
Copy link
Contributor Author

There's quite a few other internal structs with similar issues. Would it make sense to move to try and consistently use int instead of size_t everywhere? There would still be issues at boundaries with other libraries (like HDF5 and the C stdlib), but they could be handled with casts as appropriate.

@WardF
Copy link
Member

WardF commented Nov 3, 2023

Replacing size_t with int across the board isn't going to work, unfortunately; even reviewing something like that would be difficult, there could be a lot of unintended consequences. Doing this on a case by case basis will be the best approach.

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

3 participants