Re: [boost] [XInt] review

Subject: Re: [boost] [XInt] review
From: Chad Nelson (chad.thecomfychair_at_[hidden])
Date: 2011-03-09 22:22:13

On Wed, 09 Mar 2011 10:55:40 -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.

Ahh... my keyboard is safe then, it's an implementation error. :-) I've
already corrected it for the next release.

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

Then I'm not sure. They should be usable with it, but without trying it
I can't say for certain.

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

Fixed.

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

If I ever find myself with programming access to such a system, I'll
try it. They seem pretty scarce these days.

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

That took care of that problem. Any comments on the others? The second
one seems important... I had to comment all three types out of the
mpl::vector to get it to even compile, once I fixed the overloaded
functions to only work on things that are integers. As far as I can
see, that makes those tests useless.

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

Hang on... you're telling the main function to use the detail
namespace, where that function is. Wouldn't it be found directly
because of that, ignoring ADL?

After removing that using line (and modifying the options), it's still
being found though. It looks like it's because integer_t is inheriting
from things in the xint::detail namespace. As far as I can see, that
can't be eliminated without pulling more classes into the xint
namespace. Do you have a solution that wouldn't require that?
-- 
Chad Nelson
Oak Circle Software, Inc.
*
*
*