On Thu, Dec 15, 2016 at 7:53 AM, Branko Čibej <br...@apache.org> wrote:
> Some warnings on trunk have been getting on my nerves and I finally got > to the point of starting to fix them. A couple are not entirely clear-cut: > > 1. > > buckets/hpack_buckets.c:1872:27: warning: comparison of constant > 18446744073709551615 with > expression of type 'apr_uint32_t' (aka 'unsigned int') is always > false [-Wtautological-constant-out-of-range-compare] > if (v >= APR_SIZE_MAX) > ~ ^ ~~~~~~~~~~~~ > > > The relevant code is: > > { > apr_uint32_t v; > > status = read_hpack_int(&v, NULL, bucket, 5); > if (status) > continue; > > /* Send remote tablesize update to our table */ > if (v >= APR_SIZE_MAX) > return SERF_ERROR_HTTP2_COMPRESSION_ERROR; > status = hpack_table_size_update(ctx->tbl, > (apr_size_t)v); > if (status) > return status; > > ctx->state = HPACK_DECODE_STATE_INITIAL; > break; > } > > > As far as I can see, not only is the condition never true, but > read_hpack_int() already checks for overflow and returns the same status > code if it detects it. I'd suggest just removing this check, but maybe > I'm missing something. > > 2. > > buckets/fcgi_buckets.c:746:48: warning: format specifies type 'unsigned > int' but the argument > has type 'apr_size_t' (aka 'unsigned long') [-Wformat] > ctx->frame_type, ctx->stream_id, payload); > ^~~~~~~ > > > The relevant code is: > > serf__log(LOGLVL_DEBUG, LOGCOMP_CONN, __FILE__, ctx->config, > "Generating 0x%x frame on stream 0x%x of size 0x%x\n", > ctx->frame_type, ctx->stream_id, payload); > > I know APR doesn't provide a hex variant of APR_SIZE_T_FMT; on the other > hand, writing the /size/ of something to the log in hex is hardly > friendly to the poor lass who ends up having to read it. I propose > changing that size to decimal and using APR_SIZE_T_FMT. The alternative > is to use APR_UINT64_T_HEX_FMT and adding a cast. > For the payload variable, yeah, that's supposed to be logged as a decimal number instead of a hex , so agreed with your proposal. > > -- Brane > Lieven