Re: [boost] [XInt] review

Subject: Re: [boost] [XInt] review
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2011-03-10 16:08:32

AMDG

On 03/09/2011 07:22 PM, Chad Nelson wrote:
> On Wed, 09 Mar 2011 10:55:40 -0800
> Steven Watanabe<watanabesj_at_[hidden]>  wrote:
>>> 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.

Ah, I see. The test will only work if you
disable the operators directly. I expected
to see something like:

template<typename N, BOOST_XINT_CLASS_APARAMS>
typename boost::enable_if<boost::is_integral<N>, bool>::type
operator<(const integral_t<BOOST_XINT_APARAMS>& lhs, N rhs);

In general you need to be more careful about the
behaviour of operators W.R.T. overload resolution
than with most other functions, because
operators tend to be have more overloads
out of your control.

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

No. The using directive refers to the options namespace.

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

That's the solution I usually use. I just make
sure that they don't show up in the documentation.
Another option is to use an ADL barrier namespace.

namespace detail { class C; }

becomes

namespace detail {
namespace adl_barrier_for_C { class C; }
using adl_barrier_for_C::C;
}

The point is to make sure that your implementation
functions can never be found by an unqualified call
outside the library. This doesn't often cause problems,
but when it does it's really a pain to debug. I prefer
to be very strict about controlling ADL. (On that note,
it's generally a good idea to use (f)(x) or xint::f(x)
instead of f(x) unless you intend f to be a customization
point).

In Christ,
Steven Watanabe