fix: corrected type-limits and missing-field-initializers warnings
#57
Conversation
When compiling a translation unit which includes the `nbnet.h` header as
well as its implementation (`#define NBNET_IMPL`) with all warnings
(`-Wall`) and extra warnings (`-Wextra`), it spits some warnings, most
of them being of the `type-limits` kind [1], and some more being of the
`missing-field-initializers` kind [2].
[1] Most of the warnings appear in places where comparisons involving
`unsigned` data types (e.g. `unsigned int`) and the number 0. For
instance (`@@ -3987,7 +3987,7 @@`):
```c
unsigned int id;
if (id < 0 || id >= NBN_RPC_MAX)
```
In this case, checking if the value stores within an `unsigned int` is less than 0 will always return false (0), as the `unsigned` keyword specifies that the corresponding variable can only represent non-negative values, so when storing a negative value inside it it will result in actually storing a 0. As a result of this, all these comparisons are unnecessary.
[2] In the diff `@@ -1731,10 +1731,10 @@`, it's being created an array
of `NBN_Driver` with a max capacity of `NBN_MAX_DRIVERS` and initialized
with 4 instances of the aforementioned `struct`. All 4 instances are
equal, effectively assigning -1 to the `id` field of the `NBN_Driver`
struct (which is an `int` type, so it's all good). It also assigns `NULL` to the second field, but it leaves the last field "uninitialized"; in quotes because it effectively zero initializes it. The edited version of this, which fixes the warning, is to assign to all 4 `struct`'s `{ .id = -1 }`, which only assigns -1 to the `id` field, leaving all the other fields zero initialized (in case of `char *` type, it's equivalent to assign to `NULL` or to 0).
Signed-off by: iWas-Coder <wasymatieh01@gmail.com>
|
The compiler pragmas surrounding the Also, there are some attributes being defined for the Signed-off by: iWas-Coder wasymatieh01@gmail.com |
The compiler pragmas surrounding the `nbn_drivers` array are not needed anymore, due to the warnings were resolved in the previous commit (90553e5). Also, there are some attributes being defined for the `poly1305_auth` function which are undefined for GCC (clang-specific), so an aditional condition to check for that has been added. Signed-off by: iWas-Coder <wasymatieh01@gmail.com>
|
@iWas-Coder I'm glad you are enjoying nbnet. Thank you very much for this MR. It looks good but for some reason the CI did not run. I'll need to see why before merging the MR, it's probably some configuration issue in the github-actions workflow file. |
| @@ -3218,7 +3215,7 @@ void NBN_ConnectionRequestMessage_Destroy(NBN_ConnectionRequestMessage *msg) | |||
|
|
|||
| int NBN_ConnectionRequestMessage_Serialize(NBN_ConnectionRequestMessage *msg, NBN_Stream *stream) | |||
| { | |||
| NBN_SerializeUInt(stream, msg->length, 0, NBN_CONNECTION_DATA_MAX_SIZE); | |||
| NBN_SerializeUInt(stream, msg->length, 1, NBN_CONNECTION_DATA_MAX_SIZE); | |||
There was a problem hiding this comment.
what's the reason behind this change?
it seems that it fails the pipeline:
client: /home/runner/work/nbnet/nbnet/soak/../nbnet.h:3218: NBN_ConnectionRequestMessage_Serialize: Assertion msg->length >= 1 && msg->length <= 512' failed.`
it's possible to not send any data upon connection, in this case the length is 0.
| {-1, NULL}, | ||
| {-1, NULL}, | ||
| {-1, NULL} | ||
| { .id = -1 }, |
There was a problem hiding this comment.
maybe the structures should be fully initialized? I don't think the fields are necessarily going to be zero-initialized in this case.
First of all, thanks for this amazing library; I've discovered it a couple of days ago and I've been enjoying it so much, so I wanted to contribute some fixes for a couple of minor problems I've encountered along the way :D
When compiling a translation unit which includes the
nbnet.hheader as well as its implementation (#define NBNET_IMPL) with all warnings (-Wall) and extra warnings (-Wextra), it spits some warnings, most of them being of thetype-limitskind [1], and some more being of themissing-field-initializerskind [2].[1] Most of the warnings appear in places where comparisons involving
unsigneddata types (e.g.unsigned int) and the number 0. For instance (@@ -3987,7 +3987,7 @@):In this case, checking if the value stores within an
unsigned intis less than 0 will always return false (0), as theunsignedkeyword specifies that the corresponding variable can only represent non-negative values, so when storing a negative value inside it it will result in actually storing a 0. As a result of this, all these comparisons are unnecessary.[2] In the diff
@@ -1731,10 +1731,10 @@, it's being created an array ofNBN_Driverwith a max capacity ofNBN_MAX_DRIVERSand initialized with 4 instances of the aforementionedstruct. All 4 instances are equal, effectively assigning -1 to theidfield of theNBN_Driverstruct (which is aninttype, so it's all good). It also assignsNULLto the second field, but it leaves the last field "uninitialized"; in quotes because it effectively zero initializes it. The edited version of this, which fixes the warning, is to assign to all 4struct's{ .id = -1 }, which only assigns -1 to theidfield, leaving all the other fields zero initialized (in case ofchar *type, it's equivalent to assign toNULLor to 0).Signed-off by: iWas-Coder wasymatieh01@gmail.com