search

dhess / c-ringbuf

A ring buffer implemented in C
Other
418 stars 143 forks source link

Add missing header. Fix implicit cast warnings. Add missing macro. #13

Closed jamiebullock closed 5 years ago

jamiebullock commented 5 years ago

This PR includes the following:

dhess commented 5 years ago

Hi Jamie,

ISO C says that size_t is unsigned. int is signed, obviously. Therefore, I believe the (int) typecasts in your patch are not type safe as they operate on size_t types.

See here for some discussion: https://stackoverflow.com/questions/2550774/what-is-size-t-in-c

Also: I prefer that PRs are done one feature/change at a time, rather than a group of changes in one go. Separate PRs are a hassle for the submitter, but they're easier to review, and if they introduce bugs, it's easier to track down the changes/PRs that caused them. I should probably make this policy more clear in the project settings, so that contributors don't waste their time with changes that will be rejected because of this policy. I'm sorry that I didn't communicate that better.

dhess commented 5 years ago

I've added a CONTRIBUTING section to the README to make the one-feature-per-PR policy clear.

jamiebullock commented 5 years ago

Hi @dhess

Hmm... I am aware of the int / size_t issue, and on reflection I'm really not sure why I added those casts. They may have silenced a compiler warning at some point. I wonder if maybe it was related to the case of (x - y) where if y > x the result ends up causing an underflow for an unsigned type. I can see there is logic in your code to avoid this but maybe the compiler didn't used to be so smart!

I'll resubmit as individual PRs with the casts to int removed.