On Wed, 9 Mar 2011 19:11:31 +0100 Joachim Faulhaber <afojgo@googlemail.com> wrote:
This is my review on the XInt library. [...]
Thank you for the review, and the comments. I'm going to try to be brief in my replies. No offense intended, time is just constrained tonight.
1.3 Naming: Types =================
What I like about boost, compared to many other libraries, is the culture of short and concise naming.
While I can appreciate that from a typing point of view, the trend since the seventies is clearly toward more descriptive names. A name that's both short and descriptive is obviously better than one that is neither, but when conciseness interferes with descriptiveness, the general consensus seems to be that descriptiveness should win. However, typedefs offer a way to have both: a real library name that's descriptive and possibly long, and a concise version for your own use.
This includes the absence of cryptic pre and suffixes. But there are exceptions. One of those is the _t suffix used for a variety of types, namely int<n>_t types from Boost.Integer. [...]
I find that it's useful to denote types, especially when the name that you're using is the most logical one for variables of the type. But if you're referring specifically to integer_t, the problem is moot: it will be renamed to basic_integer before the next release.
2.2 Boost.Parameter considered unfavorable ==========================================
Although flexible, the options feature based on Boost.Parameter comes with an unnecessary and undesired level of abstraction and indirection. It obfuscates the different kinds of parameters and makes the implementation much more difficult to read. Moreover the meta code involved with Boost.Parameter imposes a compile time penalty. [...]
As the number of options grows, it becomes harder to use the type without Boost.Parameter or something similar. I feel that XInt has passed the threshold.
Also
integers::fixed<512> myVar; //is very intuitive, while
xint::integer_t<xint::options::fixedlength<512> > myVar; // can't probably be done without consulting the docs // and it is more verbose
Is that really a problem? You'll likely look at the docs once for each program you write, create a typedef for the integer type you want, and then ignore them entirely. Although the latter example is more verbose in this case, if you used more options it would quickly become preferable. Especially from the point of view of a reader, and code is read a lot more often than it's written.
2.3 Policy based design =======================
I think policy based design is a very good and powerful technique that is specifically useful to separate implementation variants of class templates in clear and flexible ways. Unfortunately I can not see that you are actually using policy based design here. Because your design lacks typical policy classes that encapsulate implementation variants in a way that reduces codereplication and enhances flexibility. [...]
As I'm not sure what kind of implementation you're thinking of, I can't respond to this. So far as I can tell, my design is doing what you describe for policy-based design.
There is a great deal of code replication in the current implementation of xint::integer_t. [...] This skeleton (and some variations of it) occurs over and over again only to propagate the call of the function to the implementing class at positions #1 and #2.
integer_t handles some options, and passes the data to the implementing class for others. It was the best solution I could find to handle all options.
3.2.1 Operator template overloading conflicts =============================================
In integer.hpp 2399 and 2426, there are template macros with an unrestricted template parameter 'typename N' [...]
Already dealt with for the next release. -- Chad Nelson Oak Circle Software, Inc. * * *