Re: [boost] [safe_numerics] Review Part 3 (Tests + Summary)

Subject: Re: [boost] [safe_numerics] Review Part 3 (Tests + Summary)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2017-03-08 01:23:44

AMDG

I vote to *ACCEPT* safe_numerics into Boost.

The following issues *must* be resolved before inclusion:
- Incorrect results for division (see below)
- Linker errors from multiple definition (see below)
- Name conflict for interval
- The whole mess with automatic.  (Probably either allow
  it to see the range of safe_base somehow or remove it.)
- Test cases must verify results, not just exception or not
- The precise rules for determining whether an operator
  throws must be specified.  (My preferred definition:
  An operator throws iff. it is mathematically undefined
  or the mathematical result cannot be represented in the
  result type.)

All comments are as of 3bcfabe1cafb8ef6edf08d3bd8730a1555b1c45d

The tests for the most part seem to check only
whether the operation succeeds or fails.  They
don't check whether the result is actually
correct or not.

test_z.cpp appears to be entirely #if 0'd
test0.cpp doesn't seem to do anything that
 isn't handled by the main tests.

test_divide.hpp:129:

                assert(result == unsafe_result);
unsafe_result is uninitialized.  (Most other uses are commented out)

test_values.hpp:
This is missing 0 which is kind of an important case.

I've attached my own test case for +-*/.
There's some behavior that I consider a
little odd which I noted on lines 64-72.  Also
it shows some incorrect results for division
with automatic and negative [signed] char.
for example:
generic_test.cpp(76): error: in "test_div": check base_value(f(t, u)) ==
expected has failed [1 != 0]
Failure occurred in a following context:
    -85 [safe_base<signed char,-128,127,automatic>] / 3464536379
[safe_base<unsigned int,0,4294967295,automatic>] ->
[safe_base<__int64,-9223372036854775808,9223372036854775807,automatic>]

I had to make a couple minor edits to get the library
to compile with vc15 (just released).  Patch attached.

When I include the entire library in two translation units
and link them together I get errors:

test1.obj : error LNK2005: "class std::basic_ostream<char,struct
std::char_traits<char> > & __cdecl std::operator<<<signed char>(class
std::basic_ostream<char,struct std::char_traits<char> > &,struct
boost::numeric::checked_result<signed char> const &)"
(??$?6C_at_std@@YAAAV?$basic_ostream_at_DU?$char_traits_at_D@std@@@0_at_AAV10@ABU?$checked_result_at_C@numeric_at_boost@@@Z)
already defined in include_all.obj
test1.obj : error LNK2005: "class std::basic_ostream<char,struct
std::char_traits<char> > & __cdecl std::operator<<<unsigned char>(class
std::basic_ostream<char,struct std::char_traits<char> > &,struct
boost::numeric::checked_result<unsigned char> const &)"
(??$?6E_at_std@@YAAAV?$basic_ostream_at_DU?$char_traits_at_D@std@@@0_at_AAV10@ABU?$checked_result_at_E@numeric_at_boost@@@Z)
already defined in include_all.obj
test1.obj : error LNK2005: "class std::basic_ostream<char,struct
std::char_traits<char> > & __cdecl std::operator<<<unsigned char>(class
std::basic_ostream<char,struct std::char_traits<char> > &,struct
boost::numeric::interval<unsigned char> const &)"
(??$?6E_at_std@@YAAAV?$basic_ostream_at_DU?$char_traits_at_D@std@@@0_at_AAV10@ABU?$interval_at_E@numeric_at_boost@@@Z)
already defined in include_all.obj
test1.obj : error LNK2005: "class std::basic_ostream<char,struct
std::char_traits<char> > & __cdecl std::operator<<<signed char>(class
std::basic_ostream<char,struct std::char_traits<char> > &,struct
boost::numeric::interval<signed char> const &)"
(??$?6C_at_std@@YAAAV?$basic_ostream_at_DU?$char_traits_at_D@std@@@0_at_AAV10@ABU?$interval_at_C@numeric_at_boost@@@Z)
already defined in include_all.obj

  I tried using boost::format(...) % safe<T>() and it
fails because your operator% matches.  I notice
that you have checks for ostream and istream with
the shift operators, but it would really be better
to make it more generic by checking is_arithmetic
(or perhaps std::numeric_limits::is_specialized)
on all the operators.

In Christ,
Steven Watanabe