[boost] [safe_numerics] review results

Subject: [boost] [safe_numerics] review results
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2017-03-18 22:51:28

Hi Everyone,
This is to inform you that safe_numerics library by Robert Ramey is
*CONDITIONALLY
ACCEPTED* to Boost. The list of conditions can be found down below.

I would like to congratulate Robert for getting the library into Boost, and
thank him for the effort to develop a useful library for the community. I
also would like to thank everyone who participated in the review and shared
their useful feedback.

I recorded five reviews with a yes/no vote:

   - Paul A. Bristow (yes with conditions)
   - Steven Watanabe (yes with conditions)
   - John Maddock (yes with conditions)
   - Antony Polukhin (yes)
   - Barrett Adair (yes)

And three without a vote:

   - Vicente J. Botet Escriba
   - John McFarlane
   - Peter Dimov

We also got some feedback from an individual nicknamed "scatters" at Reddit
(https://www.reddit.com/r/cpp/comments/5x1z67/boostsafe_
numerics_formal_review_starts_today/deew1kq/), and from Tomasz Kaminski in
private conversation.

There was a general consensus that the problem of surprises with C++ ints
deserves a solution. Some people observed that there are other ways to
approach the same problem, but it was agreed that the approach taken by
safe_numerics is sound.

The acceptance conditions mostly revolve around documenting micro-decisions
made by the library, and fixing bugs. They apply to the reviewed GIT commit
3bcfabe1cafb8ef6edf08d3bd8730a1555b1c45d, but I know Robert is already
working on fixing them, and has most of them fixed locally.

*Acceptance conditions:*

   - Comply with Boost formal requirements:

   1. index.html in root directory that redirects to doc/.
   2. all headers in folder include/boost/numeric/.
   3. Macro safe_literal: either remove it or let it follow Boost macro
   naming conventions:  BOOST_NUMERIC_SAFE_LITERAL().

   - Automatic promotion policy: either just remove it, or fix it so that:
      - it can see the range of safe_base in order to allow for slower
      increase in sizeof(),
      - it is clear from documentation how the promotion mechanism works.
      - in particular, what type gets selected for bit-wise operators &, |
      - The division bug (see below) is fixed.

   - Decide and document up front how the library addresses each of the
   following different situations:
      - overflow upon arithmetical operation,
      - unexpected promotion to unsigned when comparing signed with
      unsigned number,
      - arithmetic operation on unsigned int with negative value versus a
      signed value (-1 + 1u).
      - bit-wise operation on negative signed integers (unless they are
      removed), or that would produce one as the result.
      - division by zero,
      - operations upon default-constructed int type with indeterminate
      value,
      - conversion from float to int, when float contains non-integral
      value.
      - conversion from float to int, when float contains an integral value
      that would overflow upon native conversion
      - operator% where one operand is negative (for native ints it is
      implementaiton-defined)
      - leftshift by 32 bits (on 32-bit int)

   - Decide and document what safe's default constructor does.

   - Document upon which operations safe<> throws exceptions (or, signals
   arithmetic error at run-time). IOW, document in which cases safe<> never
   throws exceptions (signals run-time failure) because it is able to say that
   the operation will always succeed.

   - Document what happens when you invoke an operation on two instances of
   safe<> with different policies.

   - Resolve name conflict with interval, which already exists in namespace
   boost::numeric::interval. Either use a different namespace or a different
   name.

   - Remove anonymous namespaces from the header files. Use namespace
   detail and inline functions instead.

   - Improve test cases, so that they also check if numeric values of
   computations are correct (not only whether an operation throws or not).

   - Use enable_if (or similar) tricks to constrain the overload resolution
   for your arithmetical operations, so that they only work for numeric types
   rather than "any type T".

   - Fix the division bug:
     safe<signed, automatic> a {-1};
     safe<unsigned, automatic> b {2};
     auto c = a / b; // this returns 2147483647 <%28214%29%20748-3647>

   - Either document that safe<T> only works for scalar types (rather than
   any T that models concept Integer<T>), or provide alternate implementation
   of safe_base, as described in https://github.com/robertramey
   /safe_numerics/issues/44

   - Fix linker errors when building a program from two cpp files that
   include safe_numerics headers. The error is caused by non-inline full
   specializations of operator<<.

   - Either entirely remove the support for bit-wise ops on signed types
   (make it a compiler error), or provide a consistent implementation that
   detects any UB or surprising results.

   - `++i` should return by reference; `i--` should return by value.

*Other important issues*

The following are *not *conditions for library acceptance, but I still
recommend addressing them before the initial release.

   - Concepts: what do you need them for? Any other type except built-in
   scalars does not work anyway. Do you want to show how close safe<T>
   resembles T? In this case, they need to be changed.  If you want to extend
   safe<T> to other T's, they need to be fixed:
   https://github.com/robertramey/safe_numerics/issues/46
   <https://github.com/robertramey/safe_numerics/issues/46>
   - IOStreams interoperability needs fixes:
      - Move to a separate header, so that it does not affect compilation
      times when not needed (but provide forward declarations, so that implicit
      conversions to int do not make IOStreams work with unintended semantics)
      - Fix other problems as per https://github.com/robertramey
      /safe_numerics/pull/35
   - safe<>'s interface is not documented
   - Make the library compile with -fno-exceptions:
   https://github.com/robertramey/safe_numerics/pull/31
   <https://github.com/robertramey/safe_numerics/pull/31>
   - Some people fail to see how this library is better than just using rar
   ints but compiling with a UB-sanitizer. Address this in the documentation.
   - Prevent inadvertent ADL by putting safe into a nested namespace
   boost::numeric::save_types, and then in boost::numeric import: `using
   boost::numeric::save_types::safe`. This way  boost::numeric::safe still
   works, but inadvertent ADL of everything inside namespace boost::numeric is
   prevented.
   - Does it make sense to apply bitwise operations on two different types?
   If so, document the semantics.
   - Fix header names and paths in documentation.
   - Consider adding support for saturation policy upon overflow.
   - Expression `safe<int>{4} ^ 1` does not compile due to overload
   resolution ambiguity.
   - Fix constexpr usages.
   - If possible, make workarounds in order to make the library work with
   MSVC.
   - Document that operations on safe<> do not thorw exceptions other than
   these thrown by exception policy.

*Suggestions*
The following summarizes other useful suggestions for improving the library
in the future.

   - operation `safe<signed>{INT_MIN} + safe<unsigned>{UINT_MAX}` throws
   even though the mathematical result would fit in either signed or unsigned
   int. This is correct according to what policies describe, but is at the
   same time surprising, given that comparison
   `safe<signed>{INT_MIN} < safe<unsigned>{UINT_MAX}` just gives a
   mathematically correct answer rather than throwing an exception. Document
   this behavior explicitly.
   - Remove underflow_error from the policies: it sends the wrong message,
   that som form of an underflow is checked in this library.
   - Expose functions like add(), divide() in public interface, they might
   be useful on their own.
   - Expose the constructor with check disabled in the public interface,
   like `safe<int>{1, checks_sisabled{}}` this helps to locally disable the
   checks where the user knows that certain chain of operaitons will not
   overflow, and checks are redundant, but the library cannot see it.
   - Issues reported with bit-shift operators
   - unary operator- on unsigned type with pormotion policy `automatic`
   could return a signed type.
   - Use BOOST_NORETURN wherever possible, e.g. in throwing policies.
   - Use BOOST_LIKELY, BOOST_UNLIKELY for the policy tests.
   - Add support for statful policies.
   - Maybe drop the support for automatic type widening, to simplify the
   interface and the scope of the library, and let some other library handle
   the widening, maybe BigInt.

Regards,
Andrzej Krzemienski