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

Subject: Re: [boost] [xint] Third release is ready, requesting preliminary review
From: Chad Nelson (chad.thecomfychair_at_[hidden])
Date: 2010-05-04 20:17:59

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/04/2010 02:23 PM, 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.

That could well be, but I'll need to prove it to myself before I use
that, and I haven't had the time to do so (and probably won't in the
next few days at least, there's a lot of restructuring to do on it).

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

Perhaps I misunderstood your recommendation. But I didn't have any more
to say on the subject (I assume that anyone who doesn't know why
postincrement/postdecrement would be less efficient than their pre-
cousins and saw the warning would look it up), and the docstring is the
only thing people will see unless they specifically look at the
documentation for that function, so I believe it's better to leave it there.

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

sqr's a number, of course. ;-) Anyone familiar with sqrt(double) in the
standard library, or sqrt in XInt, should find it to be intuitive
naming. I don't expect that it will often need to be used directly anyway.

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

Noted. Easy enough to do, I'll just move the code from operator>> and
expand on it.

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

So there isn't any compiler magic that automatically re-uses the
existing allocation and just acts as a wrapper around it, for cases like
that? How disappointing. Easy enough to add that overload then.

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

I believe they are, but I also believe that the compiler is smart enough
to change them to inlined functions. It wouldn't be any major difficulty
to make that explicit though.

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

bool operator==(const integer &num1, const integer &num2) {
    return compare(num1, num2)==0; }
bool operator!=(const integer& num1, const integer& num2) {
    return compare(num1, num2)!=0; }
bool operator<(const integer& num1, const integer& num2) {
    return compare(num1, num2)<0; }
bool operator>(const integer& num1, const integer& num2) {
    return compare(num1, num2)>0; }
bool operator<=(const integer& num1, const integer& num2) {
    return compare(num1, num2)<=0; }
bool operator>=(const integer& num1, const integer& num2) {
    return compare(num1, num2)>=0; }

I think you can see the appeal of doing it that way. :-) I might be
missing a trick there, but I don't think so... compare returns as soon
as it can tell what the answer is; it only has to pursue it to the
bitter end when the arguments are equal down to the lowest digit_t. I
don't think dedicated functions for each operator could be made any more
efficient.

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

Ah, found them.

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

Good enough. Though that's a distinction that I'll never be able to
remember, so forgive any conversational lapses.

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

That seems like an arbitrary distinction.

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

As with the postincrement example, I didn't have anything further to say
on the subject -- certainly nothing requiring even a full sentence --
and "efficiently" serves as good shorthand for "more efficiently than
other commonly-used methods". It also tells the casual browser the
purpose for its existence, so he's more likely to use it for such
conversions than automatically fall back on something like lexical_cast.

>>> exceptions::too_big sounds like std::range_error to me.
>> 
>> The wording on std::range_error, in Josuttis' "The C++ Standard 
>> Library", is [...]
> 
> 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.

That, and the self-imposed constraint that the library only throw
exceptions that are declared in the boost::xint::exception namespace.

(Why? Because it seems discordant to me to have a library that only
*almost* always throws exceptions from a particular namespace, and it
was almost no additional work to make it fit my sense of rightness. In
other words, because I can. ;-) )
- -- 
Chad Nelson
Oak Circle Software, Inc.
*
*
*
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkvguTIACgkQp9x9jeZ9/wRPgQCgkKZE/XGpZ+E9HO0X8I83l/Tm
3VkAn1Rzh5Nup7l/k7ORSXWDF17T5Nug
=22Ix
-----END PGP SIGNATURE-----