Re: [boost] [safe numerics] comment

Subject: Re: [boost] [safe numerics] comment
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2017-03-08 21:40:26

Le 08/03/2017 à 19:52, Robert Ramey via Boost a écrit :
> 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.
I believe you should start by stating clearly the requirements. The 
conversion between Numerics is needed if you want to support conversion 
from safe<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.
>
> I'm convinced of this.  The concern is that implicit conversion would
> break the value checking.
I don't follow you here.
> 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;
Here you have an implicit conversion int -> safe<long> and this is safe.
> short i = l; // will be checked at runtime and be OK
Here there is an implicit conversion int -> safe<long> -> short that is 
not safe.
I believe that for a safe class, this should be forbidden at compile 
time. If the user wants to do the conversion she must be explicit

     short i = safe<short>(l);
or
     short i(l);

Think about how std::duration works. When we can loss information, 
implicit conversion are dangerous and should be avoided. This is what 
you are trying to fix with your library, isn't it?
> f(l); // conversion to short will be checked at runtime and be OK
I will expect an explicit conversion here.
>
> l = 123412312321123;
> i = l; // will throw at runtime
> f(l); // will throw at runtime
>
You say that you are convinced, but your conversions/constructors are 
implicit.

>> 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.
Could you show how?
>
>> when T and U are not the same?
> they are.  as long as the arithmetic value doesn't change.
What do you mean?
>>
>> 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.
Can you consider adding what the library supports in the documentation.
>
>> 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.
Well, I believe that soon or later some one  would need a traits that 
can be used using SFINAE.
>
>>
>> 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.
I was looking fro it here and it is not
https://github.com/robertramey/safe_numerics/tree/master/include/concept

>
>> 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.
I see these proposals as the basis for numerics. High level libraries as 
yours could be built on top of these basis.
Could you consider it?
>
> 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.

Well people have different needs. C/C++ integers are too dangerous and 
error prone. I'm sure a good safe integer library will find its users.
>
>>
>> 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, I have used to use <lib/file.hpp>, and considered "file.hpp" a 
bad style as I have seen a lot of "../..folder/file.h", but now that I 
think more on it there are also good uses of "file.hpp".

> Thanks for your comments, they are always helpful and appreciated.
>
I'm not sure I will have time to review your library in deep.

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