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-07 09:43:46

Chad Nelson wrote:
> "Stewart, Robert" <Robert.Stewart_at_[hidden]> wrote:
> > Chad Nelson wrote:
> >> "Stewart, Robert" <Robert.Stewart_at_[hidden]> wrote:
> >>
> >> 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?

I was thinking of the type on the left hand side, which you weren't showing.  Your point is valid.  However, what you've named "to_integer" could be named "integer_cast" and it would look quite reasonable:

   value = integer_cast<integer>(...)

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

In your example above, value already exists, so you get this:

   to_integer<integer>(value, ...)

Obviously, in the case of creating a new instance, it doesn't fare so well:

   integer value(...);

vs.

   integer value;
   value = to_integer<integer>(...);

Ultimately, I think the constructor is appropriate, but you might consider the function-style cast, too.

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

Calling a public member function is not accidentally misusing a class.  Casting away constness, or some other Machiavellian tactic, is abuse and there's no need to defend against that.

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

I'm confused.  integer_t<BOOST_XINT_APARAMS> cannot be a class template as it is used as a parameter type and a return type.  It is a specialization of a class template, so it is a class.  Therefore, you can create a typedef for it.

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

Now there's a shock!

> because I added the underscore because I was explicitly told that it
> *shouldn't* be part of the documented interface.

If it is a necessary feature, then it must be exposed and documented, even if needed only rarely.  Warn users away all you like, of course.

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

We apparently have a different notion of what "complete" means.

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