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