Re: [boost] [xint] Third release is ready, requesting preliminary review

Subject: Re: [boost] [xint] Third release is ready, requesting preliminary review
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2010-05-04 10:27:30

Chad Nelson wrote:
>
> The sandbox address is <https://svn.boost.org/svn/boost/sandbox/xint>;

_______________________
xint.hpp

xint.hpp surprised me.  Typically, for Boost anyway, such a header is a convenience for library users that don't wish to be more specific about the headers they include.  However, xint.hpp includes something surprising: internals.hpp.  I would have expected the other headers to have included internals.hpp if needed.

_______________________
integer.hpp

Your use of enable_if could be simplified into a single argument for the integer constructors by using mpl::and_.

The integer member function template assignment operator could benefit from the advice in <http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/>.  There may well be other member functions that should be changed as well.

integer::operator -() should not be a member function.  Only operator -=() need be.

Why is integer::operator +() defined to return *this?  That operator should return a new instance (and need not be a member).

The "not recommended, inefficient" comment for postincrement and postdecrement sounds too harsh.  It is well known that those operations are costlier than preincrement and predecrement.  You docstring can simply discuss the inefficiency in terms of memory allocations, data copying, etc. and leave the decision to the library user.  The post- operators should not be member functions.

integer::operator <<() and operator >>() should not be member functions.  Only the shift-and-assign variants need be members.

ISTR someone mentioning this before, but I'll reiterate: "odd" and "even" are not good names for predicates.  Prepend "is_" to them.

sqr() means, I suppose, square, since you have sqrt(), too.  Why not spell it out and remove doubt?

from_string() doesn't provide a strtol()-style interface in that one cannot know whether the entire string was consumed during parsing.  An overload for char const * would be appropriate so as to avoid forcing the creation of a std::string when not already available.

from_string() and from_binary() should be named "to_integer" in keeping with to_string() and because they can be overloaded.

mod(), at the least among the modulus functions, is widely applicable, so the docstring tying those functions to encryption is a bit misleading.

It looks as if you haven't taken advantage of building some of the relational operators on the others.  For example, >= is !<.  That is, many of the operators should be inlined and only a few need to be implemented.

s/template function/function template/

The docstring for parameter n in the T constructor (integer::integer(T const &, /* enable_if noise */)) should be "The value of the new instance" or something along those lines.  As it stands, the \param and \tparam text is circular.

I find it curious that the docstrings are split such that some are in the class definition and others are not.  I realize that Doxygen extracts them, but I still expect to see them in the class definition.  If you prefer them on the function definitions, then perhaps you could avoid defining any functions within the class definition.

Commentary, like "efficiently" in the to() docstring, should be left for broader library documentation.

exceptions::too_big sounds like std::range_error to me.

::operator <<() is implemented in terms of detail::operator <<() (and similar for >>).  The function in the detail namespace should probably just be called "stream" or something of that sort, don't you think?

____________________

I may look at the other headers in the near future, but there should be good food for thought in this much.

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer, Core Software      using std::disclaimer;
Susquehanna International Group, LLP  http://www.sig.com

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.