Re: [lmi] [PATCH] Fix value_cast defect shown by the unit test

[Vadim Zeitlin]

On Sun, 8 Jan 2017 17:50:08 +0000 Greg Chicares <address@hidden> wrote:

GC> Switching from boost to std is always good, especially when it removes
GC> a defect. I was tempted to replace type_traits globally throughout lmi,
GC> but feared that might collide with work you've already done.

 No, I haven't done it because I was afraid to overwhelm you with the
patches (I didn't even mean to make this one, but I really wanted to fix
the build with gcc 6 which I prefer to use as warnings/error reporting is
better in it than in previous versions). I could, of course, do it and I do
think it would be better to switch to std::stuff consistently everywhere.

GC> Then again, should I do it anyway? If you've done it separately, then
GC> comparison would show whether I make a mistake that you avoided.

 Of course, it's much more likely to be vice versa but, in any case, I
think it's relatively unlikely for mistakes to appear here as the standard
functions are identical to Boost one, so replacing one with the other is
an almost completely mechanical task.

GC> And maybe it's also time for me to do this:
GC> 
GC> http://lists.nongnu.org/archive/html/lmi/2008-06/msg00061.html
GC> | I'm thinking of writing a numeric_value_cast<T>() that doesn't
GC> | use boost at all. The original boost::numeric_cast documentation
GC> | seemed to say value preservation was an asserted postcondition:
GC> |   "An exception is thrown when a runtime value-preservation
GC> |   check fails."
GC> | but that's not true of either the old or the new boost version.
GC> 
GC> [where "new" meant boost-1.34 or -1.35],

 Which is probably the problem, as reading the documentation for the latest
version at

http://www.boost.org/doc/libs/1_63_0/libs/numeric/conversion/doc/html/boost_numericconversion/improved_numeric_cast__.html

it's definitely supposed to throw in this case:

        numeric_cast returns the result of converting a value of type
        Source to a value of type Target. If out-of-range is detected, an
        overflow policy is executed whose default behavior is to throw an
        an exception (see bad_numeric_cast, negative_overflow and
        positive_overflow). 


 Of course, it's not really difficult to reimplement this behaviour on our
own neither, so I definitely won't disagree with removing this Boost
dependency neither.

GC> >  The patch at 
GC> > 
GC> >   https://github.com/vadz/lmi/pull/50
GC> > 
GC> > contains both changes: the switch to standard functions in the first 
commit
GC> > and the fix for the problem in the second one. I've tested this with gcc
GC> > 6.3, clang (pre-4.0) and MinGW 4.9.1 used for the official builds, the 
test
GC> > now passes with all of them, including the newly re-enabled test.
GC> 
GC> Where you had proposed:
GC> 
GC> -        convertible = std::is_convertible<From,To>::value
GC> +        convertible =
GC> +               !std::is_array<From>::value
GC> +            &&  std::is_convertible<From,To>::value
GC> 
GC> I did this instead:
GC> 
GC>          convertible =
GC>                  std::is_convertible<From,To>::value
GC>              &&!(std::is_array   <From>::value && 
std::is_same<bool,To>::value)
GC>              &&!(std::is_pointer <From>::value && 
std::is_same<bool,To>::value)
GC> 
GC> I think it accomplishes all you intended, but more selectively, by
GC> disregarding undesirable conversions specifically to bool (only).
GC> I'll push this changeset in a moment.

 I agree that this version is better and can confirm that it compiles and
works with gcc 6 too, thanks!
VZ