Re: [Boost-users] [GIL] Adding base_channel_type metafunction

Subject: Re: [Boost-users] [GIL] Adding base_channel_type metafunction
From: Nathan Crookston (nathan.crookston_at_[hidden])
Date: 2010-11-07 01:39:39

On Sat, Nov 6, 2010 at 11:35 AM, Christian Henning <chhenning_at_[hidden]> wrote:
> I have done some testing to figure out if the code produced by the
> various compilers differ. But as far as I can see they produce the
> same code. What I did was to insert some local variables to check for
> their data type based on the template parameters.
>
> I don't really understand why the newer compilers don't emit warnings.
> Do you have some ideas?
I first ran into this problem several months ago, and my testing
indicated that even VC8 was usually okay with narrowing during
explicit conversion to a built-in type.  The problem seemed the
explicit conversion to the non-default type.  Perhaps MSVC is more
self-consistent in that way now?

>
>>
>> NOTE: The patch I sent earlier has an error -- in copying my changes
>> from 1.44 to the trunk I failed to replace value_type with type in a
>> couple places.  The patch attached to this email corrects that.
>>
>
> I'll apply your patch to the boost trunk and will update the release
> branch once the boost 1.45 is out.
Thanks!

>
> I like the convenience of base_channel_type and I think it makes the
> code more readable. One change I did do is to remove the typedef for
> integer_t and the cast to integer_t in
> channel_converter_unsigned_integral_nondivisible<..>. I don't think
> that's needed anymore.
Yes, I wondered about that when writing the patch.  I worried about
removing it, but I was confused by its use in the original code and
again in my patch.  It seemed to me that it could cause integer
overflow.  The following sample demonstrates the problem in the trunk
and (if I correctly understood your above comment) with your proposed
change.  In it I simply create a 30-bit channel type and convert it to
8-bit.

///BEGIN CODE demonstrating integer_t necessity & problem///
#include <boost/gil/gil_all.hpp>
#include <iostream>

namespace boost { namespace gil {
//////////////////////////////////////////////////////////////////////////
// Byte-aligned 30 bit grayscale
//////////////////////////////////////////////////////////////////////////
const uint32_t bits30_max_val = 1073741823;
struct bits30_min { static uint32_t apply() { return 0; } };
struct bits30_max { static uint32_t apply() { return bits30_max_val; } };

typedef scoped_channel_value<uint32_t, bits30_min, bits30_max> bits30;

namespace detail
{
template <>
struct unsigned_integral_max_value<bits30>
  : public mpl::integral_c<uint32_t, bits30_max_val> {};

template <>
struct unsigned_integral_num_bits<bits30> : public mpl::int_<30> {};

}//end detail

GIL_DEFINE_BASE_TYPEDEFS(30, gray)
} }//end boost::gil

namespace gil = boost::gil;

int main()
{
  gil::gray30_pixel_t g30;
  gil::gray8_pixel_t g8(255);

  color_convert(g8, g30);

  std::cout << "Value: " << g30[0] << std::endl;
  std::cout << "Should be: " << gil::bits30_max_val << std::endl;

  return 0;
}
///END CODE///

Output with trunk:
Value: 12632255
Should be: 1073741823

I have an updated patch which fixes the above problem and also
prevents a few more warnings.

Since I dislike large amounts of code in a single message I'll send
another shortly, describing how to reproduce the warning and with
another patch which should correct it and everything else discussed
thus far.

Thanks for looking over my submission,

Nate