Re: [boost] [XInt] review

Subject: Re: [boost] [XInt] review
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2011-03-09 13:55:40

AMDG

On 03/09/2011 06:13 AM, Chad Nelson wrote:
> On Mon, 7 Mar 2011 17:15:58 -0800
> Steven Watanabe<watanabesj_at_[hidden]>  wrote:
>> boost::xint::integer is totally not thread-safe, because of the race
>> on the reference count.
> I recently swore to eat my keyboard without salt if anyone found a flaw
> in the thread-safety design (rather than an implementation bug). I like
> this keyboard, but I doubt it would taste very good, especially without
> any salt.
>
> Every integer_t object will have unique storage when it's returned to
> user code (assuming the threadsafe option isn't disabled),

... by anything except the copy constructor.
The copy constructor does *not* call make_unique.

>   and no two
> threads will ever have access the same integer_t object within the
> library. With that in mind, should I get the knife and fork ready?
>

Stepping through this code in the debugger shows
copy_count=2 at the end of main:

#include <boost/xint/xint.hpp>

int main() {
     typedef boost::xint::integer_t<boost::xint::options::threadsafe> 
test_t;
     test_t test1(1);
     test_t test2(test1);
}

>> Can I use a Boost.Interprocess allocator to put an integer_t in shared
>> memory?  I doubt it since I don't see any constructors taking an
>> allocator.
> I've never tried it, but if it has the standard allocator interface, I
> believe you can. Just specify the allocator class as a parameter to the
> integer_t class template. (The documentation for that has already been
> improved for the next release.)

Interprocess has a few extra requirements
on allocators:
* The allocators are stateful.  You cannot assume that allocators
   of the same type are equivalent.
* The containers must store the Allocator::pointer type, not T*.

>> Why can the comparison operators throw? I would have expected that
>> they could be implemented to never throw.
> So far as I know, the only reason they would ever throw is if they're
> passed a Not-a-Number value, or if something unpredictable has gone
> seriously wrong.

If the integer is nothrow, then comparison doesn't throw.
Otherwise, the argument cannot be NaN.  There should
be nothing unpredictable, since you don't call user-defined
code, nor is there any memory allocation.  Exceptions
don't randomly come out of nowhere.

>> The comparison and arithmetic operators should be restricted with
>> SFINAE.  As written, they will allow comparison with a string.  Is
>> that intended?
> It would be highly unusual, but not specifically an error, assuming the
> string contains a base-ten numeric value.

I just think that the extra operators that you
allow should match the implicit conversions.
Since the constructor that takes a string is
explicit, I should have to cast the string to
an integer.

>> When you test for integer_t<BOOST_XINT_APARAMS>::Nothrow&&
>> n.is_nan(), you could just test n.is_nan() and make sure that
>> n.is_nan() is always false if Nothrow is true.
> Couldn't that prevent some less-intelligent compilers from optimizing
> out the test?

I wouldn't worry about it.  This is a fairly
easy optimization.

>> Of course this is rather hypothetical, since we always work with
>> powers of 2.
> Those types, and the originally-arcane methods used to come up with
> them, were intended to support *any* platform, no matter how odd. I've
> heard horror stories of systems with seventeen-bit words and nine-bit
> characters. :-)

I think it won't quite work, and I have little
confidence that it will ever work unless
you actually test on such a system.  It's
too easy to make hidden assumptions.

>> magnitude_t:
>> * It probably needs to be POD for you to make assumptions about the
>> layout.
> The "struct hack" is a standard C idiom, and has been for as long as C
> has been around. It only became explicitly documented behavior in C99,
> but it works with all known C and C++ compilers; I checked very
> thoroughly, and even asked on this list (where one might reasonably
> expect to find people with experience with really odd architectures and
> compiler setups), and wasn't able to find a single report of a compiler
> where it would fail.

I don't object to the 'struct hack.' I object to
using it for something that isn't a struct.

>> gcd.hpp:
>> The behavior of invmod when the inverse doesn't exist is to return
>> zero.  Wouldn't an exception/NaN be more consistant?
> I prefer to reserve exceptions for exceptional behavior, primarily
> errors. A number that doesn't have an inverse isn't really exceptional,
> there are many such values.

I think that returning 0 has the potential to be
more surprising.  From the name I would have
assumed that the postcondition of invmod is
that it returns the inverse.  If it cannot meet
this, then it should throw.  This is safer for anyone
who tries to use it without thinking about
whether the inverse exists.  If I call invmod
when there is no inverse, it is probably wrong
to go on my merry way as though the inverse
were 0.

>> I don't think std::showbase works correctly with std::setw.  (This is
>> a very common problem with ostream operators) [...]
> It looks like the standard library mostly works the way my code does,
> but has some quirks with zero values that I'm not presently emulating,
> according to
> <http://stackoverflow.com/questions/3273654/ostream-showbase-non-present-for-zero-value-and-internal-doesnt-work-for-handle>.
>

The issue is that if you split up the streaming into
multiple parts, std::setw will be applied to the first
string that you print, rather than the whole, which
kind of messes up the formatting.

>> I've written a few extra test cases. The ones called fail_*.cpp should
>> be marked as compile-fail, the ones called test should pass.
> GCC balks at some of those tests here (haven't tried MSVC yet). Maybe
> you can see why? In test_invalid_overload.cpp:
>
> Line 34 says "result.message() =", which the compiler won't accept. I'm
> not sure what is intended there.
>
> The compiler balked at lines 54 and onward ("error: no matching
> function for call to ‘boost::xint::integer_t<>::integer_t(const
> unrelated_type&)’"). No idea how to fix it, other than removing
> unrelated_type from the mpl::vector.
>
> The test_complement case reports "error: cannot pass objects of
> non-trivially-copyable type ‘class boost::xint::integer’ through
> ‘...’".
>

I only tested with MSVC 10.  Looks like gcc
is pickier about it.  Try changing the constructor
of any to be a template instead of using '...'.

> I was able to get test_arithmetic_conversion.cpp to compile and work,
> but only after changing the uniform_int_distribution.hpp include to
> uniform_int.hpp instead, and tweaking the first two lines of the
> test_arithmetic function. I'm not using the very latest Boost yet, but
> I'm using a very recent one (1.45), so I'm not sure what happened there.
>

I'm using the trunk which has some major updates to Boost.Random.
uniform_int is fine, but you'll need to specify the bounds explicitly.

> Does fail_adl_detail.cpp compile on your system? It shouldn't, so far
> as I can see, but on mine it does.

It compiles for me too.  This is one of my
favorite tests which almost always fails
unless you've thought about it.

In Christ,
Steven Watanabe