Re: [boost] [safe_numerics] Formal review starts today

Subject: Re: [boost] [safe_numerics] Formal review starts today
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2017-03-04 10:34:10

Le 02/03/2017 à 00:36, Andrzej Krzemienski via Boost a écrit :
> Hi Everyone,
> Today, on March 2nd (or tomorrow, depending on your timezone), is the start
> of the formal review of safe_numerics library by Robert Ramey. The review
> will run till March 11th. I will be serving as review manager.
>
> safe_numerics is a set of drop-in replacements for the built-in integer
> types, which make the guarantee that the results of operations are either
> correct according to the rules of integer arithmetic or report a
> diagnosable error (rather than offering modulo arithmetic results, or
> resulting in undefined behavior, or returning incorrect results as in the
> case of signed vs. unsigned comparisons).
>
> The library can be found on GitHub:
> https://github.com/robertramey/safe_numerics/
>
> Online docs:
> http://htmlpreview.github.io/?https://github.com/robertramey/safe_numerics/
> master/doc/html/index.html
>
Hi,

let me start with some comments and questions.

I was wondering if the requirements of Numeric don't need some kind of 
conversions between other numeric types.

https://htmlpreview.github.io/?https://raw.githubusercontent.com/robertramey/safe_numerics/master/doc/html/numeric.html

T(u)
T{u}

How can you implement the conversion from safe<U> to T if you can not 
convert from U to T?

We have implicit conversion from T to Safe and from Safe to T

template<class T>
constexpr /*explicit*/ safe_base(const T & t);

template<
         class R,
         typename std::enable_if<
             !boost::numeric::is_safe<R>::value,
             int
         >::type = 0
     >
     constexpr operator R () const;
constexpr operator Stored () const;

I believed that the conversion from Safe to T needed to be explicit. 
Having implicit conversion in both directions is suspect.

Why the explicit conversion between safe<T> and safe<U> are not allowed 
when T and U are not the same?

IMO, safe_base<T> is either not trivial default constructible and we 
check the validity of 0, if we don't check it, we should declare it 
=default.

The current definition doesn't adds nothing and makes the type a non-POD 
and non trivial_default_constructible, which forbids its use on binary 
messages.

     constexpr explicit safe_base() {
         // this permits creating of invalid instances.  This is inline
         // with C++ built-in but violates the premises of the whole library
         // choice are:
         // do nothing - violates premise of he library that all safe 
objects
         // are valid
         // initialize to valid value - violates C++ behavior of types.
         // add "initialized" flag.  Preserves fixes the above, but doubles
         // "overhead"
         // still pending on this.
     }

I'll suggest to use

     constexpr safe_base() = default;

BTW, why the default constructor is declared explicit?

As you say, could we talk of safe<short> when we can have one uninitialized?
I believe this needs to appear clearly on the documentation if not 
already in.

Has safe<const T> a sense?

Integer<T> are not traits as there is a compiler error when T is not an 
integer :(

template <class T>
class Integer : public Numeric<T> {
     // integer types must have the corresponding numeric trait.
     static_assert(
         std::numeric_limits<T>::is_integer,
         "Fails to fulfill requirements for an integer type"
     );
};

The same for Numeric. Have you considered to define traits (usable with 
SFINAE)?

I don't see a concept SafeNumeric. Have you considerd it?
BTW, these concept classes check only for a single requirement.

Shouldn't you specialize numeric_limits lowest()?

Have you considered to define basic algorithms managing with the valid 
conversion between Numeric with the different policies, as Lawrence 
Crowl is proposing for the standard?

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0103r1.html
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0105r1.html

I find weird the way you are including the files

     #include "concept/integer.hpp"
     #include "concept/exception_policy.hpp"
     #include "concept/promotion_policy.hpp"

     #include "safe_common.hpp"
     #include "exception_policies.hpp"

     #include "boost/concept/assert.hpp"

I believed we used to inlcude boost files using <> and the absolute path.
Is this a good practice? Does it allows the compiler to perform better?

Best,
Vicente