On Fri, 11 Mar 2011 10:12:57 -0000 "Paul A. Bristow" <pbristow@hetp.u-net.com> wrote:
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.
Thank you for your review, and your comments.
[...] A possible using-xint project (GSoC?) might be implementation of reading floating point (which require big (-ish) integers) of William D. Clinger [...] and writing FP by Steele and White [...]
So I'm not the only one who's had trouble with that. :-) Thanks for the references.
- 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?
They could, but I don't think that's really appropriate. As I mentioned in another of my all-too-numerous messages this week, there really isn't any crypto code in the library at all. There are only mathematical functions. is_prime is the closest thing to a cryptographic function, and it has non-cryptographic applications as well.
Or to Boost.Random?
Steven Watanabe has indicated that he'd rather not put any XInt-specific code into Boost.Random, and I agree. It's really more logically implemented in XInt, and it's just annoying enough to do right that I'd prefer not to force users of the library to reimplement it any time they need it.
I *really* like the *full* use of Doxygen [...] (Nevertheless, I would have much preferred to be able to get a PDF version [...]
Once the documentation is closer to its final form, I would be open to that.
I would also like more examples showing various features. These are often the quickest way for users to see what to do.
Already on the to-do list.
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.
I'm not a fan of it either. I generally only use it to help indicate that even though a function or variable might be in the public interface, it's meant for use only by the library.
3 Typo in comment This class implements the standard aribitrary-length %integer type. line 29 in integer.hpp
Fixed, thanks.
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
Puzzling... those are standard include guard lines, they shouldn't be able to give anyone any trouble.
7 I tripped on \Za - MS language extensions are essential for random (I think).
The MS language extensions are actually only essential for the Windows API headers, which are necessary for the strong_random_generator class on that platform.
I added to the jamfile to ensure this (and quiet a load of silly warnings) [...]
I've added a pragma in the code to quiet that specific warning within the XInt headers, and adopted your /Za-disabling line.
8 I noted some includes that should perhaps be <boost/xint/???.hpp> or <boost/xint/detail/???.hpp>
I'm not sure I understand, can you clarify that one? Again, thank you for the review. -- Chad Nelson Oak Circle Software, Inc. * * *