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

Subject: Re: [boost] [xint] Third release is ready, requesting preliminary review
From: Chad Nelson (chad.thecomfychair_at_[hidden])
Date: 2010-05-04 13:13:34

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/04/2010 10:27 AM, Stewart, Robert wrote:

> _______________________
> 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.

They do. I put it there thinking that it might keep at least some
compilers from looking at it again. It probably doesn't matter either
way though. I'll remove it.

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

Thanks for the suggestion, but I'd like to keep the dependencies and
compile times at a minimum. I'm also not familiar with mpl yet... I'll
look at it at some point, but I'd rather concentrate on XInt itself for
now. The time I can devote to it is limited.

> 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.

I read it when you first mentioned it (though not in detail), but I'm
deferring everything related to that until I determine whether
copy-on-write is going to be used in the final version of the library.
If it is, then passing by value is cheap in all cases, and is probably
best; if not, then it might be better to leave most functions using
const references instead, because I'm not convinced that the compiler
will have a chance to optimize the copies away in most cases.

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

The unary operator- doesn't need to be a member function, but I see no
reason why it shouldn't.

> Why is integer::operator +() defined to return *this?  That operator
> should return a new instance

Why?

> (and need not be a member).

I had unary operator- and operator+ as free functions originally, but
the compiler got confused about them when I added the fixed_integer
template type. I don't recall the details of what confused it, but
making them member functions of fixed_integer was a natural solution to
the problem, and it seemed discordant to leave the corresponding
functions for the integer and nothrow_integer types as free functions
when fixed_integer had them as member functions.

> The "not recommended, inefficient" comment for postincrement and
> postdecrement sounds too harsh. [...]

Not as harsh as not implementing them at all, which is what n1692
recommended. I prefer to implement all of the operators, and leave the
decision to the person using the library, with a warning so that he
knows to think before using them.

> The post- operators should not be member functions.

Again, why?

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

Only those variants *need* to be members, but the others *can* be
members. There's no advantage either way for the ones that I've made
members, so far as I know. Unlike the binary operator+ for example,
which potentially gains the ability to operate if the second parameter
is the type and the first parameter is convertible to it. As such, I
consider it personal preference.

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

As I said when it was recommended before, those names were recommended
in n1692. I plan to change them, since the is_ names *are* better and
I've already broken full compatibility with that document in numerous
small ways anyway, but it hasn't been a priority yet.

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

I had a lot of functions, including both of those, spelled out in
earlier iterations of the library. I changed them to match the
conventions of similar functions in the C and C++ libraries, on the
assumption that people would find them easier to remember that way.

> from_string() doesn't provide a strtol()-style interface in that one
> cannot know whether the entire string was consumed during parsing.

Not quite true. If the string contains any characters that can't be
converted, it throws an exception. If it gets to the point of returning
a value at all, then all characters were consumed.

If you need to parse out the characters that are actually part of a
number, like strtol, the stream functions will do that. At least for
base 8, 10, and 16 numbers, which I expect will be the main ones that
people will use anyway.

> An overload for char const * would be appropriate so as to avoid
> forcing the creation of a std::string when not already available.

I was under the impression that a char* would be efficiently converted
to a const std::string& when needed. If it's less efficient than I was
led to believe, then I'll certainly add an overload.

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

Hm... yes, that might be slightly better. Added to the to-do list.

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

True enough. I'll move it to the primitives.

> 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.

I'm not sure I see your point. They all consist of a single function
call and an int comparison, so the compiler will probably inline them
regardless.

> s/template function/function template/

Where do you see them? grep doesn't find the string "template function"
anywhere in the source, headers, or documentation.

And out of curiosity, do they have different meanings that I'm not aware
of, or is this just the commonly-used phrasing?

> 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.

Sure, if it's clearer to you. The circular reference seemed perfectly
clear to me.

> 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.

My rule (which I don't follow religiously) is that the Doxygen
information for individual functions is just above their
implementations, and the Doxygen information for groups of functions
(member or free) is above their declarations.

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

Why? It's far more efficient than the method that would almost certainly
otherwise be used, which is to stream it out and stream it back to the
new type, and I wanted to make that clear. I'm also not sure what
broader library documentation you're talking about.

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

The wording on std::range_error, in Josuttis' "The C++ Standard
Library", is that it's "used to report a range error in internal
computations." That didn't quite sound like what I was trying to convey
with that exception, which is that the library type that the user is
trying to convert won't fit into the type that he's trying to convert it
to. It does inherit from std::range_error though.

> ::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?

Tomayto, tomahto. :-) I used the operator names because that's what
they're solely used from, and for. It seemed clearer than giving the
version in the detail namespace a different name.

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

Thank you. Despite the objections that I raised to most of the items, I
do appreciate it.
- -- 
Chad Nelson
Oak Circle Software, Inc.
*
*
*
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkvgVbsACgkQp9x9jeZ9/wTIvgCg3ruldS4Qhx73LIecVuWqK11n
a7wAn2Lo/+H0jFDXVuUgPr1PACCRmFo0
=0QGP
-----END PGP SIGNATURE-----