Hi there, first I like to congratulate Chad for his very valuable contribution to the boost community.
3. Please explicitly state in your review whether the library should be accepted.
YES!
4. The general review checklist is provided below:
- What is your evaluation of the design?
Looks very thought out to me. No complains.
- What is your evaluation of the implementation?
I find the code pretty hard to read. Is it true that the code is cut to 80 characters a line? I'm not sure if that's a good decision. There are plenty of warnings when warning level is set to 4 on VS2010. Most of them are "warning C4127: conditional expression is constant" which seems to me that they could be removed by using compile time polymorphism.
- What is your evaluation of the documentation?
Looks very nice. Just one thing. Where can I find a list of preprocessor symbols that are used during the compilation process. For instance, I like to know more about BOOST_XINT_MOVE. The rsa example is missing the "iterator" header.
- What is your evaluation of the potential usefulness of the library?
If someone needs a bigint lib he/she should look no further. Would be interesting to see how well this lib compares to similar C libs in terms of performance and functionality.
- Did you try to use the library? With what compiler? Did you have any problems?
I used it with VS2010 in 64bit configuration.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A glance with some example code testing.
- Are you knowledgeable about the problem domain?
Not really. Regards, Christian
On Wed, 2 Mar 2011 13:06:54 -0500 Christian Henning <chhenning@gmail.com> wrote:
Hi there,
first I like to congratulate Chad for his very valuable contribution to the boost community.
Thanks! And thanks for the review too!
- What is your evaluation of the implementation?
I find the code pretty hard to read. Is it true that the code is cut to 80 characters a line? I'm not sure if that's a good decision.
Yes, I try to limit my code to 80 characters per line. You have to draw the line somewhere, and at 80 characters I can refer to documentation on a comfortably-formatted browser window while working on my code on the same monitor, and see all of it without side-scrolling.
There are plenty of warnings when warning level is set to 4 on VS2010. Most of them are "warning C4127: conditional expression is constant" which seems to me that they could be removed by using compile time polymorphism.
Odd... I tested it on VS2005 before, at the highest warning level, and those didn't come up. Now they do. Strange. In any case, those warnings are due to the design of the policy system. They're harmless noise, so I've suppressed that specific warning for the next release.
- What is your evaluation of the documentation?
Looks very nice. Just one thing. Where can I find a list of preprocessor symbols that are used during the compilation process. For instance, I like to know more about BOOST_XINT_MOVE.
The only ones that the user of the library needs to worry about are: BOOST_XINT_USE_MOVE (not documented; enable the use of Boost.Move) BOOST_XINT_NO_EXCEPTIONS (documented on the page titled "The on_exception Function And -fno-exceptions Support") Anything else is defined and used internally. If you're curious about BOOST_XINT_MOVE anyway, it's defined in boost/xint/detail/basic_types_and_includes.hpp, as either boost::move(value) (if using Boost.Move), or simply value if not.
The rsa example is missing the "iterator" header.
Sorry, I'm confused. Why is that header needed for that example? -- Chad Nelson Oak Circle Software, Inc. * * *
Hi Chad,
The rsa example is missing the "iterator" header.
Sorry, I'm confused. Why is that header needed for that example?
The rsa example uses std::back_inserter which is defined in the "iterator" header. I at least had to be able to compile the example code. I'm using VS2010. Regards, Christian
On Thu, 3 Mar 2011 10:20:45 -0500 Christian Henning <chhenning@gmail.com> wrote:
The rsa example is missing the "iterator" header.
Sorry, I'm confused. Why is that header needed for that example?
The rsa example uses std::back_inserter which is defined in the "iterator" header. I at least had to be able to compile the example code. I'm using VS2010.
Curious. It compiles fine without it on VS2005 and GCC. But it won't hurt to add it, so I have, for the next release. Thanks for pointing it out. -- Chad Nelson Oak Circle Software, Inc. * * *
From: boost-bounces@lists.boost.org wrote:
Christian Henning <chhenning@gmail.com> wrote:
The rsa example is missing the "iterator" header.
Sorry, I'm confused. Why is that header needed for that example?
The rsa example uses std::back_inserter which is defined in the "iterator" header. I at least had to be able to compile the example code. I'm using VS2010.
Curious. It compiles fine without it on VS2005 and GCC. But it won't hurt to add it, so I have, for the next release. Thanks for pointing it out.
The standard specifies which headers are required. Vendors often include one from another to pick up specific implementation curiosities, but you can't count on them. Doing otherwise makes your code non-portable as each platform will implicitly include a different set of headers. _____ Rob Stewart robert.stewart@sig.com 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.
participants (3)
-
Chad Nelson -
Christian Henning -
Stewart, Robert