Fix warning sign compare #80
Conversation
lib/include/CL/SDK/Image.hpp
Outdated
| @@ -10,7 +10,7 @@ namespace cl { | |||
| namespace sdk { | |||
| struct Image | |||
| { | |||
| int width = 0, height = 0, pixel_size = 1; | |||
| size_t width = 0, height = 0, pixel_size = 1; | |||
There was a problem hiding this comment.
Given there are changes below to reinterpret_cast these back to int in one location below is this the correct change? Should the other side of whatever was triggering the warning be changed?
There was a problem hiding this comment.
Reinterpret_cast was enforced by stb_image library which seams to be an old C-style code which uses signed int where unsigned int will suffice. As it is external library I could not change it. However Image class is a good place to set a boundary between new/old code.
Reinterpret cast is done while loading the image from file so there is no danger of loosing the data.
I would concern about writing image with stb as it is doing implicit cast from size_t to int.
There was a problem hiding this comment.
This seems like a superficial problem to me. While on an academic level I agree with "don't use signed types where values can't be negative", as ill fate, 3rd parties and legacy C compatibility may have it, sometimes a cleanup is just too much hassle.
Yes, the SDK lib chose int to interface nicely with stb. Should we change the choice of lib in the future, because the SDK utilities don't leak to users, we are always free to reevaluate that choice. Your proposed changes while seem fine at first, it is an extreme level of "running with scissors". int and size_t aren't even the same size, so casting their pointers and expecting to get the right value ventures into endianness and two's complement territory and/or knowing how much our languages (and their particular versions) shield us from them. I really don't think this part of the code, which is otherwise warning-free without turning width into a size_t is worth the hassle to reason about such matters.
Minimally, it ought to be unsigned int to make sure that stb's choice of int is always matched at least in size on every arch. Yet again, while almost every other day I'd air on the side of choosing the most restrictive yet adequate number representation, I don't think this is the case where we should practice that.
lib/include/CL/SDK/Image.hpp
Outdated
| @@ -10,7 +10,7 @@ namespace cl { | |||
| namespace sdk { | |||
| struct Image | |||
| { | |||
| int width = 0, height = 0, pixel_size = 1; | |||
| size_t width = 0, height = 0, pixel_size = 1; | |||
There was a problem hiding this comment.
This seems like a superficial problem to me. While on an academic level I agree with "don't use signed types where values can't be negative", as ill fate, 3rd parties and legacy C compatibility may have it, sometimes a cleanup is just too much hassle.
Yes, the SDK lib chose int to interface nicely with stb. Should we change the choice of lib in the future, because the SDK utilities don't leak to users, we are always free to reevaluate that choice. Your proposed changes while seem fine at first, it is an extreme level of "running with scissors". int and size_t aren't even the same size, so casting their pointers and expecting to get the right value ventures into endianness and two's complement territory and/or knowing how much our languages (and their particular versions) shield us from them. I really don't think this part of the code, which is otherwise warning-free without turning width into a size_t is worth the hassle to reason about such matters.
Minimally, it ought to be unsigned int to make sure that stb's choice of int is always matched at least in size on every arch. Yet again, while almost every other day I'd air on the side of choosing the most restrictive yet adequate number representation, I don't think this is the case where we should practice that.
|
Hi @MathiasMagnus, |
|
Merging as discussed in the September 12th teleconference. The CI failures are unrelated to these changes. |
No description provided.