-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Vladimir Prus Sent: Wednesday, March 02, 2011 10:58 AM To: boost@lists.boost.org; boost-users@lists.boost.org 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 +44 1539 561830 07714330204 pbristow@hetp.u-net.com