Re: [boost] [xint] Fifth release -- one more round of reviews, please

Subject: Re: [boost] [xint] Fifth release -- one more round of reviews, please
From: Chad Nelson (chad.thecomfychair_at_[hidden])
Date: 2010-06-13 12:53:42

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

On 06/13/2010 04:53 AM, Adam Merz wrote:

>> The changes have just been uploaded to the Sandbox (haven't updated
>> the Vault yet). Please take a look and let me know if you see any
>> problems with the implementation. There shouldn't be.
> 
> Looks good to me -- the compilation errors were solved, which was my
> only real concern. A few nitpicks though, since you asked: ;-]
> 
> Functions templated on charT taking std::basic_string<charT> should
> be templated on charT, traitsT, allocT and take 
> std::basic_string<charT, traitsT, allocT>.

Okay, done.

> xint::detail::nan_text should return 'std::basic_string<charT>
> const&' rather than just 'std::basic_string<charT>'. This way callers
> (many integer_t constructors as well as xint::to_string variants and
> stream operators) can call it like 'std::basic_string<charT> const&
> tnan = detail::nan_text<charT>();' and avoid making
> hundreds/thousands of pointless copies of the string.

That makes sense. Done.

> For functions taking 'charT const*, charT**', this is a bit C-ish;
> obviously this is pedantic/stylistic concern, but charT*& is
> preferred over charT**.

It's a bit C-ish because it was based on a C function. :-) There's no
similar function that would confuse the compiler (or the user), so I've
changed it, but I'm ambivalent about it... having to add the address-of
operator was a clue that it was an out-parameter. Without it, you can't
tell that it is unless you look at the source code or documentation. A
minor quibble though.

> The functions templated on charT are fine, but for the functions
> specifically overloaded on std::string/std::wstring (e.g. to_string,
> to_wstring), consider also providing additional overloads for
> char16_t and char32_t based on BOOST_NO_CHAR16_T and
> BOOST_NO_CHAR32_T.
> 
> to_string and to_wstring have identical implementations except for
> the character type they hardcode in. Refactor these into a function
> template so there's a single common implementation, and have outer
> functions defer to the template.

I've made a new template function, to_stringtype, that takes a type to
make the string from. to_string and to_wstring are now just thin shells
around it.

I'm not sure how often the other two types will be used (and I doubt
anyone is at this point). I'm also not sure what I'd call those
functions... to_char16_string seems esthetically unpleasant. It's the
kind of thing that, if I ran across it in an otherwise well-designed
library that I wanted to use, I'd write a function or macro to rename it
and hide its ugliness.

So I propose leaving it as it is. If someone needs that capability,
to_stringtype will provide it, and they can write whatever wrapper
function or macro they want around it.

Combining messages...

> integer_t constructors taking 'const charT *, charT **' don't
> compile, due to const correctness issues (assigning a charT const* to
> a charT*). convert.hpp line 140 works around it with const_cast, but
> the signature should just be changed to 'const charT *, const charT
> **' (or better yet 'const charT *, const charT *&', as previously
> mentioned).

I wondered about that, but I guess I got distracted before I could find
a satisfying fix for it. In any case, it's fixed now.

> raw_integer line 131 & convert.hpp line 103, from_string takes char**
> rather than charT**.

Oops... sorry, I was in a bit of a hurry, and there was no test to point
that out. Both of those problems are now fixed.

> std::size_t should be used universally instead of size_t.

As a defensive measure, it makes sense, though I'd question the sanity
of any developer who made his own different size_t type. Done.

These changes have been uploaded to the Sandbox now. If you have the
time to spare, please check them and ensure that I haven't made a
mistake. And thanks for the feedback, I really appreciate it.
- -- 
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/

iEYEARECAAYFAkwVDRMACgkQp9x9jeZ9/wSfdwCgqDsSmDfXiL/YkeWRkQP6ve4J
sJ4An3BMnuwx0UFCD7nvJfVOup2EpCFA
=XxGb
-----END PGP SIGNATURE-----