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-11 10:29:52

On Fri, 11 Mar 2011 10:12:57 -0000
"Paul A. Bristow" <pbristow_at_[hidden]> 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.
*
*
*