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

Subject: Re: [boost] [xint] Boost.XInt formal review
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2011-03-05 14:15:45

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.

> >> - What is your evaluation of the design?
> >
> > I'm concerned about the application of Boost.Move and the
> > continued reliance on COW.  I don't know Boost.Move, so I
> > cannot comment on whether it was employed correctly or
> > completely, but enough concern has been expressed that I'm
> > left uncomfortable.
>
> 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.

> > 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.  You could also pass the instance as a non-const reference argument to allow for type deduction.

> > There are a number of implementation details brought into
> > the public access section of integer_t, such as
> > xint::detail::integer_t_data<BOOST_XINT_APARAMS>::data.
> > Those details should be in the private access section.
>
> 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).

> > 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, but I cannot imagine a template parameter list hidden behind such a macro would preclude creating a typedef.

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

> > There is a great deal of duplication among the string-based
> > constructors.  Those functions should reuse a common
> > implementation to avoid code bloat.
>
> I'm sorry, I don't understand. The parts that can be factored
> out have been, to the BOOST_XINT_RAWINT::from_string functions,
> where two of the three overloads call the third to do the
> common work.

They all have the same structure, including try blocks, conditionals, etc.  There are certain differences, to be sure, but those can be factored out into more specific overloaded member functions.  The result is that they can be implemented using a member function template, as an example.

> > An application that only uses integer_t<options::nothrow>
> > instances, there is a lot of code in those else branches
> > that isn't needed.  Far better would be for no_throw_integer
> > to derive from integer_t.
>
> I had that in previous iterations, and was told that since
> they were so similar, I had to make them all a single class if
> I wanted the library to be accepted.

Well, whatever you had before and whatever the reasons for it, the result doesn't strike me as likely to be better.  I'm in no position to judge between the two seeing only this version, of course.

> > Code like the following appears in most member functions.
> > Each such case should be factored out into a member
> > function:
> >
> >   if (Threadsafe == true) { r._make_unique(); data.make_unique(); }
>
> If it were made into a member function, it couldn't be
> trivially detected as dead code and eliminated in variants that
> don't use it, and some compilers might not be smart enough to
> follow the function call. That's a feature, not a bug. :-)

I don't think an inline function will thwart dead code elimination.  I really hate to see such duplication, but you have to maintain it.

> >> - What is your evaluation of the documentation?
> >
> > The documentation is the weakest part of this library.  There is no
> > helpful flow introducing the concepts in the library.
>
> 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.

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

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

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

> 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

> > integer_t(const integer_t<...> & b, bool
> force_thread_safety = false)
> >
> > The description is "This is an overloaded member function,
> > provided for convenience. It differs from the above function
> > only in what argument(s) it accepts." [...]
>
> That wording comes from Doxygen, when you tell it that a
> function is an overload via the \overload command.

Then I suggest not using \overload.

> > [...] 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..."

> > integer_t::hex_digits(), is_even(), is_odd(), sign()
> >
> > The description states that, "The nothrow version returns
> > zero instead of throwing."  There's no mention of any
> > exception being thrown. Under what circumstances might that
> > happen? [...]
>
> Not-a-number, for instance. And any object can throw an out-of-memory
> exception.

The point is that the documentation should not leave such questions open.

_____
Rob Stewart                           robert.stewart_at_[hidden]
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.