Re: [boost] [Review] XInt Review

Subject: Re: [boost] [Review] XInt Review
From: Chad Nelson (chad.thecomfychair_at_[hidden])
Date: 2011-03-04 10:04:44

On Fri, 04 Mar 2011 10:49:25 +0100
Joel Falcou <joel.falcou_at_[hidden]> wrote:

> Here is my review for XInt. Currently I think the library should not
> be included in boost as a lot of problems are, in my opinion, still to
> be fixed and the design has to be rethought.

Thank you for the review. I'll answer some of your comments below.

> The premises are OK. We have some large integer type that has proper 
> value semantic and can be used in a variety of operations and
> functions. So far, so good. The main culprit is that the current
> library design is rather hardwired to work with a choice of
> implementation the author thought to be OK at some point but which
> still have to be proven better than existing solution.

What existing solution? So far as I can see, no one else has been
willing to do the work needed to offer one, only criticize, condemn,
and complain about the one I've offered.

> The overall design lacks separation between the data and the algorithm
> that makes the library difficult to extend: what if i want to take an 
> arbitrary range of integers and look at them as a huge series of
> bits ?

Why would you? Give me a viable use-case, and if it fits the library's
design criteria, I'll be happy to try to make it happen.

> What if i don't want the extra memory overhead so i can mmap some
> files directly into memory (cause reading a 10k bits integer is a
> no-no) ?

Use the to_binary function.

> Can I pass the integer type to a STD algorithm if I want to iterate
> over the bytes it contains ? or copy them using boost::range copy ?

No, you cannot. Deliberately. If you were able to, then the interface
would be permanently stuck at a version-1 state, because if I made any
future changes to it, it would break existing client code. In my
opinion, the freedom to improve the internal design without breaking
client code is far more desirable than giving client code free rein to
mess around in its internals.

> On the data side, i think being forced to use a given representation
> is wrong. The Ideal should be that the library provide a fall back 
> implementation but provide adaptor to adapt any kind of data to be 
> looked at as a large integer.

Your opinion has been noted. The code is not ideal, but it works, it's
available, and it can (and will, if not rejected) be improved. If
you're waiting for the perfect library to come along before you'll
accept it, you're going to be waiting forever.

> The definition of a LargeInteger concept would help defining such an
> adapting interface and make the library more fluid. For example, what
> if i want to implement LCM ?

Point A, you don't have to, LCM is already in there. Point B, it can
handle essentially any mathematical operation that can be expressed as
a combination of existing operations. No one has yet provided any
concrete example of a mathematical operation that *can't* be done using
the operators and functions already available, and I suspect you'd have
to get pretty esoteric to find one.

> - There is a lot of unwarranted pass-by-value function prototype
> which seriously impact performance even with COW. pass-by-ref >
> pass-by-value + COW.

I've already said that I plan to change this, several times.

> Moreover, the "thread safety" COW is dubious.

Then you're don't understand it.

> - the headers are obscenely big. It could be easier to follow the
> code it there was not this 2k+ line mammoth files.

Yes, I could break it up into 2K one-line files. Somehow I think that
would be even harder to follow.

> In its current form, the library *should not* be accepted into Boost.
> performances claims are not backed up (and other reviewers show it
> can be far better0 the design lacks of genericity and its
> implementation is , in my opinion, sub-optimal.

Would it be easier on everyone if I just deleted that line, since most
people are bikeshedding about it?

> To be accepted the following point should be addressed: [...]
>
> - do real benchmarks and compare to existing state of the art 
> solution, at least show benchmarks against GMP on realistic use cases.

GMP is a collaboration of dozens of developers, contains years of work,
and includes low-level optimizations that go against XInt's design
goals. XInt is the product of one developer over five months. Do you
measure a child's running speed against that of an Olympic gold
medalist?
-- 
Chad Nelson
Oak Circle Software, Inc.
*
*
*