View Issue Details

IDProjectCategoryView StatusLast Update
0005573GNUnetutil librarypublic2019-02-28 11:17
ReporterschanzenAssigned ToChristian Grothoff 
PrioritynormalSeveritytrivialReproducibilityalways
Status closedResolutionfixed 
Product VersionSVN HEAD 
Target Version0.11.0Fixed in Version0.11.0 
Summary0005573: dnsparser warning
DescriptionWe 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 ReproduceThe 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 (?)
TagsNo tags attached.

Activities

Christian Grothoff

2019-02-15 23:05

manager   ~0013801

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.

Christian Grothoff

2019-02-15 23:05

manager   ~0013802

(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!)

schanzen

2019-02-16 09:11

developer   ~0013811

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?

Christian Grothoff

2019-02-16 10:53

manager   ~0013812

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.

Christian Grothoff

2019-02-16 10:53

manager   ~0013813

caa65b8a2..c902a0d22

Issue History

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 => SVN HEAD
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