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

Silence sign conversion warnings from NClist functions #2812

Merged
merged 4 commits into from
Jan 19, 2024

Conversation

ZedThree
Copy link
Contributor

Fixes warnings about sign conversion from uses of nclistget/nclistlength and other NClist functions.

In a lot of the places that these functions are used, the index is an int instead of size_t, mostly in loops. In this PR, I've fixed these local variables to be size_t -- an alternative would be to change the API for NClist to use int, but unlike #2781 this still leaves >100 warnings, so there would still be lots of places that need fixing.

I've opted for changing the local variables to size_t rather than just casting the argument to nclistget, as this tended to fix multiple warnings in one go. There are a few places where casting is the better option, mostly where there's some interaction with a netCDF int ID.

There was one place (libdap2/getvara.c) where I also changed the function signature, but this was a local static function and I could verify all places where it is used.

I've left alone any place where there's some potentially unsafe arithmetic done with the list index, e.g. index - 1 or index--, mostly in loops iterating backwards over the list. I just wanted to go for the easy, low hanging fruit here.

This silences >400 warnings.

Copy link
Collaborator

@DennisHeimbigner DennisHeimbigner left a comment

Choose a reason for hiding this comment

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

I think there is an error in this PR somewhere. When run thru github actions,
several tests are timing out.

@ZedThree
Copy link
Contributor Author

I was a little rushed and missed a couple of places where it wasn't safe to just switch to size_t because e.g. some arithmetic was done with the result of nclistlength -- now fixed.

There's a few places where it would be safe to switch to size_t but then return/break if nclistlength(list) == 0. I've not made those changes here to try to keep this large PR simpler.

@ZedThree
Copy link
Contributor Author

ZedThree commented Jan 15, 2024

A much simpler alternative could be to change size_t to int in nclist.h/c completely. This changes ~20 lines and would silence ~250 warnings, although it might be then necessary to insert some >= 0 checks in places.

@WardF
Copy link
Member

WardF commented Jan 19, 2024

Testing something locally; I expect it to pass, but better safe than sorry. Thanks, once this passes I'll get it merged in!

@DennisHeimbigner
Copy link
Collaborator

might be then necessary to insert some >= 0 checks in places.

Definitely true in any function in nclist that is checking an incoming index into the list.

WardF added a commit that referenced this pull request Jan 19, 2024
@WardF WardF merged commit 16bcb1d into Unidata:main Jan 19, 2024
99 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.

3 participants