Re: [boost] [safe numerics] comment

Subject: Re: [boost] [safe numerics] comment
From: Robert Ramey (ramey_at_[hidden])
Date: 2017-03-08 18:52:01

On 3/8/17 9:36 AM, Robert Ramey via Boost wrote:
> I'm re-posting Vicente's message which apparently got lost in a server
> issue so we can respond to it.
>
> Robert Ramey
>
>
> 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}

Hmmm - I see that safe types have this. My intention is that the Numeric 
concept include both built in types as well as safe versions of same. 
Do built in types like int support this?  That is is int(42) legal? 
I'll look into this.

> How can you implement the conversion from safe<U> to T if you can not
> convert from U to T?
I believe this would fail - probably with some totally confusing error
message.  I think the best would be:

a) Improve implementation of Numeric and other concepts with static_asserts.
b) Use these implementations to check types when used.

> 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.

I'm convinced of this.  The concern is that implicit conversion would
break the value checking.  But I don't see that as a concern as
value checking is implemented by overloading the binary assignment 
operator and the casting operator.  So

void f(short x){
     ...
}
safe<long> l = 42;
short i = l; // will be checked at runtime and be OK
f(l); // conversion to short will be checked at runtime and be OK

l = 123412312321123;
i = l; // will throw at runtime
f(l); // will throw at runtime

> Having implicit conversion in both directions is suspect.
> Why the explicit conversion between safe<T> and safe<U> are not allowed
I believe they are.  implicit conversions as well.

> when T and U are not the same?
they are.  as long as the arithmetic value doesn't change.
>
> 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;

OK - I'll try this.

> BTW, why the default constructor is declared explicit?
> As you say, could we talk of safe<short> when we can have one
> uninitialized?

I think this is cruft - I'll check
>
> I believe this needs to appear clearly on the documentation if not
> already in.
>
> Has safe<const T> a sense?

Hmmm - I'm not sure.  what about - safe<T &> and safe<const T&>.  Truth 
is I haven't considered this. I've used const safe<T> to good effect.

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

I'm aware of this.  In this library Integer<T> etc. are used only
for type checking and a compiler error is just fine for that.  I don't
even want to think about using this for code dispatch now.  In the past 
I've used BCC for this but i think it's out dated and somewhat awkward. 
It produces a compile time error as well.  For those who need 
concepts-lite style concept checking I recommend Paul Fultz's Tick 
Library to be found in the incubator.  (off topic) Actually, I don't 
think that this library is more than sufficient for those who actually 
need such a facility.  I've used it on other projects.

> 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 didn't need them.  What I did need and used is to create some traits
"is_safe" and maybe a couple of others - off hand I don't remember.

>
> I don't see a concept SafeNumeric. Have you considerd it?

Hmmm it's in the documentation.  It's implemented in the code (using BCC 
concept checking!)  But I didn't actually use it in the code.  As I 
mentioned before, I think I should use these concepts in the code to 
help with user decipher error messages.  Given this and my earlier
comments, I would convert this to static_assert.

> BTW, these concept classes check only for a single requirement.
> Shouldn't you specialize numeric_limits lowest()?

Right - good call.  And they should check other requirements such as 
is_integer.

> 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 hadn't seen these - they are spanking new!  I gave a presentation to 
the committee on safe numeric library a year ago.  I don't know if this 
is related in any way. My impression of the response was that it was 
underwhelming. Oh - I see now that the are revisions of previous papers 
which I had seen before.  My general impression of these efforts are

a) that are not user friendly for fixing existing code.
b) they don't exploit modern C++ facilities which permit many issues to 
be addressed a compile time.

safe numerics is much more practical for users to use.  But it's also 
much more complex to implement.  I asked Bjarne at his presentation at 
CPPCon 2016 about his view of Boost.  His response included the comment 
that he thought many boost library were overly complex.  I would 
certainly agree that many boost libraries are too complex to be in 
something like a C++ standard.  This is one of the main reasons I'm 
pivoting toward boost and away from the inclusion in the standard.

>
> 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.

LOL - this has been mentioned multiple times.  My reasons for this is
the following:

My understanding is that <> when one intends to search all the include 
directories in the list of include directories specified on the command 
line and/or some environmental variable (INCLUDE?).  Otherwise one 
can/should use "" if one doesn't need to do that.  The test/build 
examples don't need to do that so they use "".  One advantage is that I 
don't have to worry about accidental collisions with names in other 
directories which i forgot are in my include list.

> Is this a good practice? Does it allows the compiler to perform better?
This has not been a consideration for me. It might be faster at compile 
time, but I'm doubtful it would be detectable.

Thanks for your comments, they are always helpful and appreciated.

Robert Ramey

>
> Best,
> Vicente
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>