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-05 10:09:28

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

On 05/05/2010 07:56 AM, Stewart, Robert wrote:

>>>>> The "not recommended, inefficient" comment for postincrement and
>>>>> postdecrement sounds too harsh. [...]
> [snip]
>> 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.
> 
> I expected that you'd discuss the actual overhead of the post- 
> operators so users could make an informed decision. Yes, we all know 
> that the post- operators are less efficient, but in your case,
> because of the memory allocation and large amount of data that may be
> copied, it can be worse than one might assume, because of the work
> your class must do.

It seems obvious to me, but I may well be too close to it to see the
problem. I'll add some further information.

>>>>> sqr() means, I suppose, square, since you have sqrt(), too.  Why
>>>>> not spell it out and remove doubt?
> [snip]
>>> 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.
> 
> I still don't understand.  Is "sqr" an alternative spelling of "sqrt"
> or does it mean "square?"  I know of sqrt(), but I've never seen
> sqr() before, so your assertion is false.  I found no manpage for it
> and a cursory search of the Internet turned up nothing on it.

It's not needed in the C/C++ library, since it's just as fast to
multiply the numbers together when using single-precision integers. I
used that name to fit in with the style of the other libraries. But it
doesn't matter now; I've had multiple recommendations to change it, so I
will.

[...]
>> 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.
> 
> Those are trivial, and all based upon a single function, so you do 
> get the reuse to which I referred. However, there is an inherent 
> inefficiency here, though it may be miniscule relative to the
> comparison code. A compare() function must return one of three
> values: less than zero, zero, or greater than zero. That implies
> additional logic that isn't necessary when computing <, ==, etc.
> (Splitting those apart means duplicating portions of the logic in
> compare(), of course.)

It's quite minuscule. I'm no CPU hardware expert, but my educated guess
would be two additional clock cycles per comparison at most. I'll take
that cost over the cost of duplicating the logic in multiple functions,
especially as it's not trivial.

> BTW, should you implement compare() using memcmp()? I would expect 
> that function to be optimal as it is usually implemented in
> assembler and takes great advantage of platform characteristics.

Unfortunately, memcmp works on bytes, not digit_ts. Since the digit_t
type is presumably more than one byte, it would only work correctly on
big-endian systems.

>>> 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.
> 
> I contend that "efficiently" is meaningless then. It's a QoI detail,
> your textual claim has no effect on the code, and it's not quantifiable.
> It certainly doesn't tell me that one operator is to be preferred over
> another because the one is described as "efficient" and the other isn't.

I disagree, but again, I may be too close to it to see the problem. I'll
add a more expansive explanation.

>>>>> exceptions::too_big sounds like std::range_error to me.
> [snip]
>>> 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. ;-) )
> 
> That's a nice rationale. Be sure to put it in your docs (assuming it
> isn't).

It isn't, yet. I didn't think it was something that required
justification. But it's easy enough to add.
- -- 
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/

iEYEARECAAYFAkvhfBgACgkQp9x9jeZ9/wREDACfXf0Z5wUtbUcEuzOH7FrHnyhD
yWoAoMjMYQ44klKza0wOyHpnhSY2JSsT
=6WZJ
-----END PGP SIGNATURE-----