Re: [boost] [serialization][1_42_0] basic_binary_oarchive - warning: comparison is always true due to limited range of data type

Subject: Re: [boost] [serialization][1_42_0] basic_binary_oarchive - warning: comparison is always true due to limited range of data type
From: Kim Barrett (kab.conundrums_at_[hidden])
Date: 2010-04-06 23:43:17

On Apr 6, 2010, at 1:29 PM, Robert Ramey wrote:
> Jeff Flinn wrote:
> > basic_binary_oarchive.hpp has this member:
> > 
> > void save_override(const class_id_type & t, int){
> >  // upto 32K classes
> >  assert(t.t <=
> >  boost::integer_traits<boost::int_least16_t>::const_max); const
> >     boost::int_least16_t x = static_cast<const 
> > boost::int_least16_t>(t.t); * this->This() << x;
> > }
> > 
> > It doesn't seem that there is a need for this runtime assert at all
> > given the definition:
> > 
> >  BOOST_ARCHIVE_STRONG_TYPEDEF(int_least16_t, class_id_type)
> > 
> > Can't the assert be removed to quiet the gcc warning?
> 
> Hmmm - what about a platform where int_least16_t is typedefed as a 32
> bit integer?  That is where int_least16_t can hold a value greater
> than 32K?

The test is still tautological, since it is asserting that a value of
type T is not greater than the maximum value for the type T (where T
in this case is int_least16_t). On such a platform the value of
boost::integer_traits<boost::int_least16_t>::const_max would be the
maximum value for that 32 bit integer type. There are several
similarly tautological assertions nearby.

One possibility is that the test bound that is actually wanted here is
boost::integer_traits<boost::int16_t>::const_max so that 32K actually
is the limit.  Or perhaps instead it should be
boost::integer_traits<short>::const_max, which is at least 32K, sticks
to a primitive type, and is stylistically consistent with the
assertion for the save_override for version_type, which uses unsigned
char as the limiting type. Though the latter will likely still lead to
the gcc warning at issue.

Note that I ran into this same warning recently as part of upgrading
to boost 1.42. Some research led me to write the following comment in
my local patch:

    // kab, 4/2/2010: workaround for gcc warnings:
    //   comparison is always true due to limited range of data type
    // gcc 4.3 and later provide warning controls for this: controled by
    // -W[no-]type-limits, defaulting to disabled and enabled by -Wextra.
    // gcc versions prior to that provide NO CONTROL over this warning, which
    // first appeared circa gcc 3.3.2. see gcc bug 12963.

Frankly I'm hard-pressed to ever think of a situation where I'd want
to enable this warning, but unfortunately there's no way to disable it
for a significant range of gcc versions.

For expedience I changed these asserts to use boost.numeric.conversion
facilities to perform the range check, as it carefully optimizes out
tests guaranteed to succeed, and so avoids triggering the warning in
question. I'm guessing that a real patch that Robert would accept
would avoid adding the dependency on boost.numeric.conversion, but
that was good enough for my immediate purposes. Specifically, my patch
introduces the following helper function:

  template<class T, class S>
  bool save_override_check(S s) {
      return boost::numeric::cInRange
             == boost::numeric::converter<T, S>::out_of_range(s);
  }

which is used as

  assert(save_override_check<boost::int_least16_t>(t.t));