On Mon, 7 Mar 2011 13:06:10 -0800 (PST) Artyom <artyomtnk@yahoo.com> wrote:
There is my formal review of Boost.Xint.
Thank you for the review, and the comments.
Please explicitly state in your review whether the library should be accepted
YES.
However, I expect that algorithms performance would be improved ASAP.
Definitely. I've already made some changes that should help, for the next release.
Cons/Problems: -------------- Points I think that are wrong:
- Move Semantics:
It is not clear, and not documented!
That's because it's presently not used. The code is all there for it, but it's disabled because the future of Boost.Move was still up in the air when I wrote it. To turn it on, define BOOST_XINT_USE_MOVE before including any XInt headers. It will be on by default as soon as Boost.Move is in the Boost distribution, which I expect will be before XInt is ready to be added.
boost::xint::integer a,b; a = 100; b = boost::move(a); // expect like swap(a,b) acts like a=b std::cout << a<<" " <<b << std::endl; // expect 0 100, get 100, 100
integer is container and should behave like container in move context and like like a primitive type - mostly for optimization.
I haven't tried it, but that code should work the way you expect with the move code enabled.
- Copy between different types (bug of feautre?):
integer<opt::secure> a;
// works integer<opt::threadsafe> b; a=b;
// works integer<opt::threadsafe> b(a);
// Does not! integer<opt::threadsafe> b = a;
It seems that copy constructor explicit without any reason.
The reason was that different integer types may well have different behaviors -- the behavior of the library when assigning a negative value, for example, differs depending on the options specified. Making that constructor explicit makes it harder for users to get blindsided by those differences.
- What is your evaluation of the implementation?
I can't judge the code as I do not so familiar with such complicated meta-programming but I have following notes:
Style: ------
1. The coding is too dense. Complex code requires much more comments especially when it is not trivial.
Entirely possible. I try to use self-documenting code wherever I can, but sometimes I forget that what's obvious to me won't necessarily be obvious to others.
2. There are lots of template classes and subclasses and other relations. I think internals should be documented otherwise in a year or two nobody would understand what is going on here.
So good white paper about internals should be given.
Once the internals are pretty much settled, that can be done.
Random number generation under Linux -------------------------------------
It is just a bug... I had this one in my code once so, not a negative point but just something small to fix: [...] By default lots of bytes are going to be read due to buffering - very inneficient
Sheesh... I'd heard about that problem too, and I still forgot it when I wrote that code. Thanks for pointing it out.
Performance: ------------
This is the weakest point of this library, the performance of too many algorithms is very low [...]
A lot of that may be due to pass-by-value instead of const references, rather than the algorithms, which I've already corrected for the next release. I'll be testing that soon, maybe today, along with the CoW/move speeds.
Some algorithms should be rewritten with the best known solutions. [...]
Already on the list.
Finally with all COW/Threasafty/Atomics the actual algoriths were "forgotten".
Not entirely. Jeffrey Hellrung pointed out that the square-root algorithm I'm using is suboptimal, and John Bytheway introduced me to a new primality algorithm that I plan to add. But the algorithms used are really secondary to the design. Algorithms can be improved internally.
Bottom Line and Vote: ---------------------
YES, it should be included in boost.
However I have a request (almost condition): --------------------------------------------
First problem that should be solved are actually algorithms complexities, I don't think it is something too hard. Reading few books and papers would guide a strightforward direction to the solution. [...]
I'll report on the speed changes from the alterations I've already made as soon as I have them. I don't want to check any code into Subversion until the review is over (the reviews should all be of the same code), but I can make the updated code available earlier if anyone wants it.
Additional Recommendation: --------------------------
[...] - Documentation should be improved with much more examples.
I'll see what I can come up with. Thank you again. -- Chad Nelson Oak Circle Software, Inc. * * *