Boost logo

Boost :

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
>


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk