View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0005573 | GNUnet | util library | public | 2019-02-15 21:15 | 2019-02-28 11:17 |
| Reporter | schanzen | Assigned To | Christian Grothoff | ||
| Priority | normal | Severity | trivial | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Product Version | Git master | ||||
| Target Version | 0.11.0 | Fixed in Version | 0.11.0 | ||
| Summary | 0005573: dnsparser warning | ||||
| Description | We get this in compile: dnsparser.c:1091:25: warning: result of comparison of constant 65535 with expression of type 'const enum GNUNET_DNSPARSER_CertType' is always false [-Wtautological-constant-out-of-range-compare] if ( (cert->cert_type > UINT16_MAX) || ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~ dnsparser.c:1092:25: warning: result of comparison of constant 255 with expression of type 'const enum GNUNET_DNSPARSER_CertAlgorithm' is always false [-Wtautological-constant-out-of-range-compare] (cert->algorithm > UINT8_MAX) ) ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~ | ||||
| Steps To Reproduce | The respective struct members are enums. So from the view of the compile those are tautologies. However, I assume from the spect those values are 16 and 8 bits respectively. We should handle this (?) | ||||
| Tags | No tags attached. | ||||
|
|
That's just our code being conservative against future changes, after all, the enum could eventually have > 255 or >65535 values. That should be ignored IMO. |
|
|
(or ideally, we'd want the opposite: the build failing if the warning is *not* being generated, but I don't know how to do that!) |
|
|
Those two goals (seem to) contradict themselves. If you want to be conservative against future changes, then the check seems odd unless _in this specific case_ the values must be within that range. In that case, however, I would argue that we use a dedicated define. On the other hand, if we want to opposite, failing of is the warning is not triggered, what you actually want is just simply making the struct members 8 and 16 bit, respectively. Instead of an enum. Then the compiler will give you the expected warning as soon as the enum exceeds the bounds, right? |
|
|
Changing the CertRecord to use 8/16 bit members would work here when converting to dcert, but be missguided when we use it elsewhere as without the 'enum' any 'switch' statement would no longer benefit from warnings of unhanded cases. My point was: if ever somebody adds an enum value outside of the allowed range, that would be very bad (that's why we have the current check!). At that point the warning would disappear (dynamic check could be true), but what we'd want (in a perfect world) is *then* to get a hard error instead of the current runtime error. Anyway, I'll put in some #defs to make clang shut up. |
|
|
caa65b8a2..c902a0d22 |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2019-02-15 21:15 | schanzen | New Issue | |
| 2019-02-15 21:15 | schanzen | Status | new => assigned |
| 2019-02-15 21:15 | schanzen | Assigned To | => Christian Grothoff |
| 2019-02-15 23:05 | Christian Grothoff | Note Added: 0013801 | |
| 2019-02-15 23:05 | Christian Grothoff | Note Added: 0013802 | |
| 2019-02-15 23:06 | Christian Grothoff | Severity | minor => trivial |
| 2019-02-15 23:06 | Christian Grothoff | Status | assigned => feedback |
| 2019-02-15 23:06 | Christian Grothoff | Product Version | => Git master |
| 2019-02-16 09:11 | schanzen | Note Added: 0013811 | |
| 2019-02-16 09:11 | schanzen | Status | feedback => assigned |
| 2019-02-16 10:53 | Christian Grothoff | Note Added: 0013812 | |
| 2019-02-16 10:53 | Christian Grothoff | Status | assigned => resolved |
| 2019-02-16 10:53 | Christian Grothoff | Resolution | open => fixed |
| 2019-02-16 10:53 | Christian Grothoff | Fixed in Version | => 0.11.0 |
| 2019-02-16 10:53 | Christian Grothoff | Note Added: 0013813 | |
| 2019-02-16 10:53 | Christian Grothoff | Target Version | => 0.11.0 |
| 2019-02-28 11:17 | Christian Grothoff | Status | resolved => closed |
