Re: [boost] [xint] Third release is ready, requesting preliminary review

Subject: Re: [boost] [xint] Third release is ready, requesting preliminary review
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2010-05-04 14:23:25

Chad Nelson wrote:
> On 05/04/2010 10:27 AM, Stewart, Robert wrote:
>
> > The integer member function template assignment operator could
> > benefit from the advice in
> > <http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/>.
> > There may well be other member functions that should be changed as
> > well.
>
> I read it when you first mentioned it (though not in detail), but I'm
> deferring everything related to that until I determine whether
> copy-on-write is going to be used in the final version of the library.
> If it is, then passing by value is cheap in all cases, and is probably
> best; if not, then it might be better to leave most functions using
> const references instead, because I'm not convinced that the compiler
> will have a chance to optimize the copies away in most cases.

If the compiler elides the argument copying, then the COW logic won't be invoked.  It's a win with or without COW.

> > integer::operator -() should not be a member function.
> > Only operator -=() need be.
>
> The unary operator- doesn't need to be a member function, but I see no
> reason why it shouldn't.

Scott Meyers wrote on that some years ago.  The principle is to keep everything out of the class that you can so that there's less to inspect whenever you alter the class' implementation.  That is, if a non-member operator -() is built atop operator -=(), and you change the class implementation details, you only need to check for effects on operator -=().

> > Why is integer::operator +() defined to return *this?  That operator
> > should return a new instance
>
> Why?

Um, because that operator is used for i + j and returns the sum, right?  Did I miss something?

> > (and need not be a member).
>
> I had unary operator- and operator+ as free functions originally, but
> the compiler got confused about them when I added the fixed_integer
> template type. I don't recall the details of what confused it, but
> making them member functions of fixed_integer was a natural
> solution to
> the problem, and it seemed discordant to leave the corresponding
> functions for the integer and nothrow_integer types as free functions
> when fixed_integer had them as member functions.

There's no harm in making those operators members, but the class definition shrinks which makes maintenance easier when they aren't.  In this case, as a means to overcome what confused the compiler, at least as things stood at some point in time, I can understand your tack.

For binary operators, you frequently need to implement some as non-members to allow for swapped argument order, so it makes sense to implement both variants as non-members in terms of the one assignment variation which must be a member.

> > The "not recommended, inefficient" comment for postincrement and
> > postdecrement sounds too harsh. [...]
>
> Not as harsh as not implementing them at all, which is what n1692
> recommended. I prefer to implement all of the operators, and leave the
> decision to the person using the library, with a warning so that he
> knows to think before using them.

I think part of what you elided was a suggestion that information regarding the inefficiency be put where you can give it fair treatment, not in a terse, somewhat misleading docstring.  I also noted that it should be the library user determining whether the cost is worthwhile.

> > sqr() means, I suppose, square, since you have sqrt(), too.  Why not
> > spell it out and remove doubt?
>
> I had a lot of functions, including both of those, spelled out in
> earlier iterations of the library. I changed them to match the
> conventions of similar functions in the C and C++ libraries, on the
> assumption that people would find them easier to remember that way.

What does sqr() do?

> > from_string() doesn't provide a strtol()-style interface in that one
> > cannot know whether the entire string was consumed during parsing.
>
> Not quite true. If the string contains any characters that can't be
> converted, it throws an exception. If it gets to the point of
> returning a value at all, then all characters were consumed.
>
> If you need to parse out the characters that are actually part of a
> number, like strtol, the stream functions will do that. At least for
> base 8, 10, and 16 numbers, which I expect will be the main ones that
> people will use anyway.

I'd prefer to have both forms.  That is, if I call the overload which takes a non-const reference to an iterator or size_t, I can determine where the parsing left off and not get an exception.  If I assume everything should parse, I'd use the current overload.

> > An overload for char const * would be appropriate so as to avoid
> > forcing the creation of a std::string when not already available.
>
> I was under the impression that a char* would be efficiently converted
> to a const std::string& when needed. If it's less efficient than I was
> led to believe, then I'll certainly add an overload.

std::string construction from a char * implies free store allocation and copying.

> > It looks as if you haven't taken advantage of building some of the
> > relational operators on the others.  For example, >= is !<.
>  That is,
> > many of the operators should be inlined and only a few need to be
> > implemented.
>
> I'm not sure I see your point. They all consist of a single function
> call and an int comparison, so the compiler will probably inline them
> regardless.

I didn't see the implementation of those, so I assumed that they were declarations of non-inlined functions.  Nevertheless, this is another case of only having to worry about the implementation of a core set of functions and leave the others in terms of those.

> > s/template function/function template/
>
> Where do you see them? grep doesn't find the string "template
> function" anywhere in the source, headers, or documentation.

Search case insensitively.  I think it was "Template function" actually.

> And out of curiosity, do they have different meanings that
> I'm not aware
> of, or is this just the commonly-used phrasing?

The correct terminology is "function template" and "class template."  IOW, they are templates for generating functions or classes, they are not functions or classes.

> > Commentary, like "efficiently" in the to() docstring, should be left
> > for broader library documentation.
>
> Why? It's far more efficient than the method that would
> almost certainly
> otherwise be used, which is to stream it out and stream it back to the
> new type, and I wanted to make that clear. I'm also not sure what
> broader library documentation you're talking about.

First, I'm assuming that Doxygen generated documentation will be for reference and not for tutorial, rationale, etc. content.  That is the "broader library documentation" to which I refer.  Second, when describing what a function does, the word "efficiently" isn't helpful.  If you prefer, you can discuss the efficiency aspects in the rest of the docstring (not part of the brief line).

> > exceptions::too_big sounds like std::range_error to me.
>
> The wording on std::range_error, in Josuttis' "The C++ Standard
> Library", is that it's "used to report a range error in internal
> computations." That didn't quite sound like what I was trying
> to convey
> with that exception, which is that the library type that the user is
> trying to convert won't fit into the type that he's trying to
> convert it
> to. It does inherit from std::range_error though.

The name fits nicely, even if the description suggests something slightly different.  Arguably, your internal computation determined that the value exceeded the range of the target type, so the exception type is fitting.

In the end, there's no harm in having your own type, given that it does derive from range_error.  It allows calling code to be more specific if warranted.

> > ::operator <<() is implemented in terms of detail::operator
> > <<() (and
> > similar for >>).  The function in the detail namespace should
> > probably just be called "stream" or something of that sort,
> > don't you think?
>
> Tomayto, tomahto. :-) I used the operator names because that's what
> they're solely used from, and for. It seemed clearer than giving the
> version in the detail namespace a different name.

I just think calling operator functions by name to be awkward so I usually avoid it.

> Thank you. Despite the objections that I raised to most of
> the items, I do appreciate it.

You're welcome.  I didn't expect you to agree with everything, least of all the stylistic suggestions.

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