On Sat, 5 Mar 2011 14:15:45 -0500 "Stewart, Robert" <Robert.Stewart@sig.com> wrote:
Chad Nelson wrote:
On Thu, 3 Mar 2011 12:33:16 -0500 "Stewart, Robert" <Robert.Stewart@sig.com> 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.
Sorry again, but I knew if I didn't either make the changes you suggested immediately, or at least enter them into my issue tracker, some of them would get lost. And I kept getting distracted answering other messages... I'd actually been working through your message since a few hours after you sent it.
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.
If he's interested, certainly.
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.
Would it? You'd still have to specify what integer type you want the to_integer function to return, wouldn't you?
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.
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).
In recent years, I've come to the conclusion that the Python philosophy (<http://stackoverflow.com/questions/70528/why-are-pythons-private-methods-not-actually-private/70555#70555>) has a point in that respect: regardless of public or private, if someone *really* wants to get to your class's data, he will find a way to do so. So "private" is really just a convention, a warning not to use something, probably because no attempt will be made to preserve compatibility with it in future versions. 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.
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.
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, because I added the underscore because I was explicitly told that it *shouldn't* be part of the documented interface.
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.
I've put it on the to-do list, I'll see if I can come up with something.
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.
Certainly. I said that because you know what you'd like to see, and I can only guess at it. Anything I come up with without guidance will obviously be different from what you'd intended, maybe enough that it wouldn't suit your intent at all.
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.
I'm sorry, it wasn't my intent to disparage your contribution. Your suggestions are good, and I've implemented almost all of them, or noted them for later implementation. I simply feel that "complete" is accurate, especially once the improvements are finished. And given the time I spend on it, I believe "carefully maintained" is justified as well.
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. :-)
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
Ah, okay. Fixed.
[...] 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..."
Maybe it was the move constructor. In any case, they all have unique descriptions now. Again, thank you for your comments. -- Chad Nelson Oak Circle Software, Inc. * * *