Re: [boost] [xint] Boost.XInt formal review

Subject: Re: [boost] [xint] Boost.XInt formal review
From: Chad Nelson (chad.thecomfychair_at_[hidden])
Date: 2011-03-08 11:17:04

On Mon, 7 Mar 2011 13:06:10 -0800 (PST)
Artyom <artyomtnk_at_[hidden]> wrote:

> There is my formal review of Boost.Xint.

Thank you for the review, and the comments.

>> Please explicitly state in your review whether the library should be
>> accepted
>
> YES.
>
> However, I expect that algorithms performance would be improved ASAP.

Definitely. I've already made some changes that should help, for the
next release.

> Cons/Problems:
> --------------
> Points I think that are wrong:
>
> - Move Semantics:
>
>   It is not clear, and not documented!

That's because it's presently not used. The code is all there for it,
but it's disabled because the future of Boost.Move was still up in the
air when I wrote it.

To turn it on, define BOOST_XINT_USE_MOVE before including any XInt
headers. It will be on by default as soon as Boost.Move is in the Boost
distribution, which I expect will be before XInt is ready to be added.

>        boost::xint::integer a,b;
>        a = 100;
>        b = boost::move(a);  
>        // expect like swap(a,b) acts like a=b
>        std::cout << a<<" " <<b << std::endl;
>        // expect 0 100, get 100, 100
>
>  integer is container and should behave like container in move
>  context and like like a primitive type - mostly for optimization.

I haven't tried it, but that code should work the way you expect with
the move code enabled.

>- Copy between different types (bug of feautre?):
>
>  integer<opt::secure> a;
>
>  // works
>  integer<opt::threadsafe> b;
>  a=b; 
>
>  // works
>  integer<opt::threadsafe> b(a);
>
>  // Does not!
>  integer<opt::threadsafe> b = a;
>
>  It seems that copy constructor explicit without any reason.

The reason was that different integer types may well have different
behaviors -- the behavior of the library when assigning a negative
value, for example, differs depending on the options specified. Making
that constructor explicit makes it harder for users to get blindsided
by those differences.

>>         - What is your  evaluation of the implementation?
>
> I can't judge the code as I do not so familiar with such complicated
> meta-programming but I have following notes:
>
> Style:
> ------
>
> 1. The coding is too dense. Complex code requires much more comments
>    especially when it is not trivial.

Entirely possible. I try to use self-documenting code wherever I can,
but sometimes I forget that what's obvious to me won't necessarily be
obvious to others.

> 2. There are lots of template classes and subclasses and other
> relations.
>    I think internals should be documented otherwise in a year or
>    two nobody would understand what is going on here.
>
>    So good white paper about internals should be given.

Once the internals are pretty much settled, that can be done.

> Random number generation under Linux
> -------------------------------------
>
> It is just a bug... I had this one in my code once so, not a negative
> point but just something small to fix: [...] By default lots of bytes
> are going to be read due to buffering - very inneficient

Sheesh... I'd heard about that problem too, and I still forgot it when
I wrote that code. Thanks for pointing it out.

> Performance:
> ------------
>
> This is the weakest point of this library, the performance of too
> many algorithms is very low [...]

A lot of that may be due to pass-by-value instead of const references,
rather than the algorithms, which I've already corrected for the next
release. I'll be testing that soon, maybe today, along with the CoW/move
speeds.

> Some algorithms should be rewritten with the best known solutions.
> [...]

Already on the list.

> Finally with all COW/Threasafty/Atomics the actual algoriths were
> "forgotten".

Not entirely. Jeffrey Hellrung pointed out that the square-root
algorithm I'm using is suboptimal, and John Bytheway introduced me to a
new primality algorithm that I plan to add. But the algorithms used are
really secondary to the design. Algorithms can be improved internally.

> Bottom Line and Vote:
> ---------------------
>
> YES, it should be included in boost.
>
> However I have a request (almost condition):
> --------------------------------------------
>
> First problem that should be solved are actually algorithms
> complexities, I don't think it is something too hard. Reading few
> books and papers would guide a strightforward direction to the
> solution. [...]

I'll report on the speed changes from the alterations I've already made
as soon as I have them. I don't want to check any code into Subversion
until the review is over (the reviews should all be of the same code),
but I can make the updated code available earlier if anyone wants it.

> Additional Recommendation:
> --------------------------
>
> [...] - Documentation should be improved with much more examples.

I'll see what I can come up with.

Thank you again.
-- 
Chad Nelson
Oak Circle Software, Inc.
*
*
*