[boost] [safe_numerics] review results

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 a {-1}; safe 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