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

Subject: Re: [boost] [xint] Boost.XInt formal review
From: Paul A. Bristow (pbristow_at_[hidden])
Date: 2011-03-11 05:12:57

> -----Original Message-----
> From: boost-bounces_at_[hidden] [mailto:boost-bounces_at_[hidden]]
> On Behalf Of Vladimir Prus
> Sent: Wednesday, March 02, 2011 10:58 AM
> To: boost_at_[hidden]; boost-users_at_[hidden]
> Subject: [boost] [xint] Boost.XInt formal review
> 
> Boosters,
> 
> the formal review of the Boost.XInt library, by Chad Nelson, starts now
and will
> run through March 11.

> 3. Please explicitly state in your review whether the library should be
accepted.
=================================================================

Although there is work to do, I think this will be a useful Version 1.  So I
vote for acceptance.

In view of the several skeletons on the Boost road to a BigInt, I think it
would be unfortunate to risk losing this one.

I would like to see a user base and applications,  and more testing on more
platforms.

A possible using-xint project (GSoC?) might be implementation of reading
floating point (which require big (-ish) integers) of William D. Clinger

http://www.cesura17.net/~will/Professional/Research/Papers/howtoread.pdf

http://portal.acm.org/citation.cfm?id=93557

and writing FP by Steele and White

http://grouper.ieee.org/groups/754/email/pdfq3pavhBfih.pdf

I am sure that there are many others.
 
What is your evaluation of the design?
=====================================

It seems to have most of the features needed, 'fixed' and 'elastic'.

I am not qualified to judge the 'presentation' (or the implementation), but
from the discussions, I am absolutely clear that it is not at all simple.

Some conflicts between runtime speed, space, expression templates,
movability, compile time, COW, CRTP, thread-safety and others seem
inevitable.  A single 'ideal' version may even prove impossible.  Macros to
switch implementation are a messy solution but may be the only way.

I do not believe we are anywhere near a C++ Standard candidate model, so
this is not yet a consideration for me.

I would not rule out Xint2, SlickerInt or even UltimateInt, because I
believe the many differences of views expressed can only be resolved by
seeing some more persuasive evidence.

Since GMP is a 'gold standard' (but, for some, useless because of license
limitations) I highly value correctness above speed or space (and the
unfettered Boost License terms).
Portability and correctness are of paramount importance for a Boost library.

<aside>

I feel this review shows that our process is seriously broken.  IMO there
has been too little recognition of the hard work put in by the author, and
too much noisy asserting and hand-waving arguments which are not
sufficiently persuasive.  Since there has already been considerable
discussion about Xint some time ago, if people feel the design is
fundamentally wrong, I feel they should put much more effort into at least
sketching out their 'XintBetter' and working to provide evidence of the pros
(and cons) of their suggested implementation.

There are far too few reviews, especially those with hard experience of
using the library 'in anger'.

The review process is taking place over far too short a time period, with no
time to attempt to take suggestions on board and trying to implement them.
For a difficult issue like this, we are not going to get to a good (let
alone the best) solution on the first iteration.

Why has no reviewer actually tried to use it with UBlas, ET ...?  There is
much speculation,
not all with the marvellous modesty of Eric Niebler "But I haven't thought
it all through, so that could all be bollocks. :-)".  We could use  a lot
more of that attitude? 

We have too little evidence of portability - libraries probably need to be
tested in the Boost Test System to get a good idea of all platforms and
compilers. *We need this before review*, not just testing a single GCC and
VS 2008.

</aside>

>         - What is your evaluation of the implementation?

As has been suggested already, I think the random and cypto applications are
muddying the water at this stage.  They could perhaps be moved to the
/example folder for now?  Or to Boost.Random?

>         - What is your evaluation of the documentation?

OK.

Too chatty in places.
Some of the "Why would I use it" should go.

I *really* like the *full* use of Doxygen to document each function and
class with parameter description, returns, exceptions ...  (Too many
otherwise well-documented Boost libraries do not use Doxygen documentation
system to the full:  you get the structure and names, but finding what they
*do* is much more difficult, and to often impossible.).  

(Nevertheless, I would have much preferred to be able to get a PDF version
(implies using Quickbook with Doxygen and Autoindex toolchain - becoming a
Boost 'standard').  If the library is accepted, I would encourage the author
to use the existing material to create this.  It would be a small extra task
(re-)using everything done so far but will make the docs more widely
available and more useful too (and help can be given).

I would also like more examples showing various features.  These are often
the quickest way for users to see what to do.

What is your evaluation of the potential usefulness of the library?
======================================================

Essential - for some tasks.

But so also is 'XReal'  - an even bigger can of worms :-(

Did you try to use the library?
=======================

VS 2010.  Ran the tests and an example OK.

Tests look OK - though I am sure that more could be added, including more
corner cases and perhaps spot values from other packages.  More comment on
the specific 'difficult' case being tested would be helpful?

How much effort did you put into your evaluation? 
===========================================

A quick try-out and read-through.

Are you knowledgeable about the problem domain?
==========================================

No.

Paul

Some minor comments.
===================

It is not possible (or even desirable) to try to *prevent* access to
implementation details.  Putting them in a /details folder and documenting
their status is sufficient at this stage.

1  _test 

I've not a fan (nor is it is Boostish) to start function names with _.  Some
people use the MS-style int m_x; for member data, and others use _x or x_
for member data.  But that's a style issue.

2  A jamfile to build all the examples would be helpful?

3 Typo in comment This class implements the standard aribitrary-length
%integer type. line 29 in integer.hpp

4  I tried to run the jamfile, but it failed not finding
boost/xint/integer.hpp

5 I converted the msvc sln  to VS 2010 (OK)

and added a property page with an include directory for "boost-sandbox/xint"

but still got trouble with

#ifndef BOOST_INCLUDED_XINT_INTEGER_HPP
#define BOOST_INCLUDED_XINT_INTEGER_HPP

These should perhaps be

#include <boost/xint/detail/internals.hpp>
#include <boost/xint/random.hpp>

6 Personally I'd like to see the output from examples appended as a comment
like this:

/*

The number is: 72
The huge number is: 123456789012345678901234567890123456789012345
(That's a 147-bit number.)
Press any key to continue . . .

*/

7  I tripped on \Za - MS language extensions are essential for random (I
think).

I added to the jamfile to ensure this (and quiet a load of silly warnings)

project  
    : requirements 
      <toolset>msvc:<cxxflags>/wd4127 # expression is constant.
      -<toolset>msvc:<cxxflags>/Za # Ensure language extensions are enabled
(essential only for random?)
    ;

8  I noted some includes that should perhaps be <boost/xint/???.hpp>
or <boost/xint/detail/???.hpp>

---
Paul A. Bristow,
Prizet Farmhouse, Kendal LA8 8AB  UK
[phone]  07714330204
pbristow_at_[hidden]