Re: [boost] [xint] Boost.XInt formal review

Subject: Re: [boost] [xint] Boost.XInt formal review
From: Chad Nelson (chad.thecomfychair_at_[hidden])
Date: 2011-03-05 20:14:40

On Sat, 5 Mar 2011 14:15:45 -0500
"Stewart, Robert" <Robert.Stewart_at_[hidden]> wrote:

> Chad Nelson wrote:
>> On Thu, 3 Mar 2011 12:33:16 -0500
>> "Stewart, Robert" <Robert.Stewart_at_[hidden]> wrote:
>>
>> (I apologize for getting to this message out of order, but it
>> had a lot of information and I wanted to give it the attention
>> it deserved.)
>
> Hah!  I was just planning to search gmane for my post thinking it was
> lost.

Sorry again, but I knew if I didn't either make the changes you
suggested immediately, or at least enter them into my issue tracker,
some of them would get lost. And I kept getting distracted answering 
other messages... I'd actually been working through your message since
a few hours after you sent it.

>> Unless I'm forgetting something (which is entirely possible,
>> given my mental state over the last few days), no one has
>> provided any concrete suggestions or criticisms about my use
>> of Boost.Move.
>
> It may be that too much else has precluded a serious look.  Anyway,
> when you remove the excess copying and disable COW to focus on
> performance tuning, that should reveal a lot about using move
> semantics.  You might even ask Ion Gaztañaga to look over the code at
> that point.

If he's interested, certainly.

>>> The integer_t(const charT * str, std::size_t base = 10) and
>>> similar constructors seem out of place.  Free functions that
>>> return an integer_t seem more appropriate and would be in
>>> keeping with the strtoX() functions.
>>
>> I tried various forms of that in earlier incarnations. The
>> results varied between aesthetically displeasing and downright
>> ugly. [...]
>>
>> Since I had to_string and to_binary, a set of to_integer
>> functions would have been ideal. But to *what* integer? The
>> type is a template with many options. The best I could come up
>> with would have required client code like this:
>>
>>     value = to_integer<integer>(...)
>>
>> Making those constructors eliminated the need to specify a type
>> parameter to a function, allowing this instead:
>>
>>     value = integer(...)
>>
>> Why force the person using the library to type both every time?
>
> That is certainly an issue.  auto obviates the problem, of course.

Would it? You'd still have to specify what integer type you want the
to_integer function to return, wouldn't you?

> You could also pass the instance as a non-const reference argument to
> allow for type deduction.

Possible, though it would require at least two lines instead of one
(creating the integer object, then passing it to the to_integer
function). Painful, from an aesthetically point of view.

>> If they were, I'd have to make a lot of other functions in the
>> public interface into friends. A publicly-available, but
>> obviously-for-internal-use-only _data function seems the
>> lesser evil.
>
> I prefer to ensure that clients cannot misuse a class even if that
> means using friendship (which can be constrained with, for example,
> the Advocate idiom).

In recent years, I've come to the conclusion that the Python philosophy
(<http://stackoverflow.com/questions/70528/why-are-pythons-private-methods-not-actually-private/70555#70555>)
has a point in that respect: regardless of public or private, if
someone *really* wants to get to your class's data, he will find a way
to do so. So "private" is really just a convention, a warning not to
use something, probably because no attempt will be made to preserve
compatibility with it in future versions. 

I definitely agree with making it hard to *accidentally* misuse a
class. But trying to make it impossible is probably misguided. Fences
and big warning signs around high-voltage power stations are good;
hermetically sealed domes are likely overkill.

>>> All references to integer_t<BOOST_XINT_APARAMS> in the
>>> integer_t class template definition should be replaced with
>>> a typedef to improve clarity in the source.
>>
>> Is that possible? I'm pretty sure I tried it, in an attempt to
>> de-uglify the documentation, and the compiler wouldn't accept
>> any form of it. The BOOST_XINT_APARAMS macro, and similar
>> ones, were the best compromise I could come up with. If you're
>> referring to using a #define instead, as I've done with some
>> of the internal template classes, that would cause Doxygen to
>> show useless information.
>
> I don't know what's in BOOST_XINT_APARAMS, of course,

A lot of opaque Boost.Parameter stuff. Or an ellipses, when compiling
the documentation.

> but I cannot imagine a template parameter list hidden behind such a
> macro would preclude creating a typedef.

I haven't tried it recently, but *can* you typedef a template? Not a
concrete class made from a template, but the template itself, with
parameters? If so, I'd dearly love to know the syntax. That's what I
was trying to find a way to do, and why I finally had to settle on
BOOST_XINT_APARAMS.

>>> Why is _make_unique() named as it is?  The leading
>>> underscore suggests private/implementation detail, but it is
>>> clearly public, so the underscore is just odd and confusing.
>>
>> Like _data, it is meant for use by library functions only, as a
>> less-evil alternative to making all of them friend functions.
>
> I disagree with your assignment of relative evil to those things.
> Being able to misuse a class is worse than using friendship.  However,
> in this case, it seems that the ability to dissociate an instance's
> memory is an important feature that will arise in other algorithms and
> even, in some cases, in client code.  If this feature remains
> necessary -- presumably because of the retention of COW -- then this
> function should be public, even if it requires care to use reasonably.

Then it seems that there's some disagreement in the Boost community,
because I added the underscore because I was explicitly told that it
*shouldn't* be part of the documented interface.

>> Unfortunately, the majority of concepts are either trivial to any
>> reader with a grade-school education (addition, multiplication,
>> etcetera), or far too complex to provide any sort of tutorial for in
>> a library (like prime number theory). If you can point out any in
>> between, I'll be happy to document them, I'm too close to the subject
>> to necessarily know what's not trivial.
>
> I'd expect you to show how xint::integer can be used in a normal
> calculation then lead the reader through some variations of the
> options and their effect on usage.  I'd expect you to motivate reasons
> for the various tools through more specific examples, even partial
> ones, leading through the existing examples, of course.  The that you
> don't walk a reader through the documentation in steps introducing
> features little by little.

I've put it on the to-do list, I'll see if I can come up with something.

>> If you'll provide a point-by-point outline of what you'd like
>> to see, I'll be happy to write it.
>
> I might manage the time to do so, but I must point out that it is your
> library and not mine.

Certainly. I said that because you know what you'd like to see, and I
can only guess at it. Anything I come up with without guidance will
obviously be different from what you'd intended, maybe enough that it
wouldn't suit your intent at all.

>>> Why would I use it?
>>>
>>> Bullet 3 deviates from the preceding text by using a
>>> staccato style with sentence fragments and self-describing
>>> the documentation as "complete and carefully maintained" is
>>> excessive. [...]
>>
>> I spend a great deal of time on the documentation, and not to
>> put too fine a point on it, it's an order of magnitude better
>> than that of even a few accepted Boost libraries (not naming
>> any names, I don't have the time to contribute patches to them
>> so I don't think complaining about them is fair). "Complete
>> and carefully maintained" is, in my biased opinion, accurate,
>> despite the current limitations you've identified (and which
>> I'll be remedying).
>
> This reply discourages my desire to offer further help.

I'm sorry, it wasn't my intent to disparage your contribution. Your
suggestions are good, and I've implemented almost all of them, or noted
them for later implementation. I simply feel that "complete" is
accurate, especially once the improvements are finished. And given the
time I spend on it, I believe "carefully maintained" is justified as
well.

> The many questions about behavior raised by the reviews and discussion
> should clearly show that the documentation is not yet complete.

There's always room for improvement, even on "complete"
documentation. :-)

>> In the RSA example...
>>
>>> The code indentation makes the access sections hard to spot.
>>
>> I'm not sure what you mean by "the access sections," could
>> you clarify?
>
> public, protected, and private

Ah, okay. Fixed.

>> > [...] a user that wishes to understand this particular
>> > constructor must find the other to which you refer in order
>> > to get a proper description.  Please duplicate all such
>> > descriptions rather than rely on such indirection.
>>
>> Sorry, but I'm confused yet again. All of the constructors already
>> include unique descriptions. The \overload command is there only to
>> draw attention to the fact that there are other functions that share
>> that name.
>
> The entire description I saw was, "This is an overloaded..."

Maybe it was the move constructor. In any case, they all have unique
descriptions now.

Again, thank you for your comments.
-- 
Chad Nelson
Oak Circle Software, Inc.
*
*
*