Re: [boost] [review] Review of Nowide (Unicode)

Subject: Re: [boost] [review] Review of Nowide (Unicode)
From: Artyom Beilis (artyom.beilis_at_[hidden])
Date: 2017-06-22 20:43:03

On Wed, Jun 21, 2017 at 3:18 AM, Groke, Paul via Boost
<boost_at_[hidden]> wrote:

> * I'm a big fan of "try" names, so I'd suggest to rename the array versions of widen/narrow to try_widen/try_narrow. Makes it more clear that they can fail without throwing. Also I'm not sure about the benefit of returning the output buffer pointer or NULL. I'd simply return bool instead.
>

These aren't try functions as they succeed in general (unless you are
out of memory after conversion policy update) and returing char
*/wchar_t* makes code shorter.

> * nowide_filesystem... to be honest I'm not sure what this does. If it does something related to UTF-8 on POSIX systems, then I smell a potential problem. If it's only for wide->narrow on Windows, it should be OK.
>

Probably documentation should be improved.

Boost.Filesystem uses std::codecvt facet to convert between narrow and
wide encodings. On windows the default is ANSI encoding. By starting
integration it installs utf8/utf-16 codecvt facet forcing
boost::filesystem to use utf-8 in its narrow strings on Windows.

> * Also not sure what boost::nowide::cin/cout/cerr do. Do they call SetConsoleCP/SetConsoleOutputCP? Will they work with re-directed input/output streams? What will happen if the user uses them AND std::cin/cout/cerr or stdin/stdout/stderr in the same application?

SetConsoleCP is problematic and does not allow you to input UTF-8.
What it does it uses native console Wide API to write/read to/from
console:

http://cppcms.com/files/nowide/html/index.html#technical_cio

In case of redirection it just becomes alias of std::cin/cout (docs
need to be updated a little)

>
> * I'm not a big fan of the basic_stackstring classes. I'd suggest to provide template function overloads for narrow/widen instead. Like:
> users_favorite::wstring_class wstr;
> if (boost::nowide::try_widen(wstr, str))
>     use(wstr);
> else
>     too_bad();
> They could write to the target-string using the usual duck-typing - e.g. via clear/push_back or clear/back_inserter. That way the user can use whatever SSO string class he likes and Nowide wouldn't have to provide string classes -- which IMO isn't its job. As a bonus, the user also gets an exception-free (except bad_alloc) function that doesn't require him to provide a raw array output buffer.
>

These are mostly internal functions and since the "target" is WinAPI
you need to provide finally a pointer to string. So I don't see
advantage in iterator based approach,

> * The Windows & invalid UTF-16 thing... I still think invalid UTF-16 should round-trip.
>
>> - What is your evaluation of the implementation?
>
> Haven't had much time to check the implementation. What I did notice is that when windows.h is not used, the WinAPI prototypes are declared in the global namespace. This can cause issues when windows.h is also included before/after the Nowide headers, and the prototypes don't match exactly (e.g. short vs. wchar_t). Also it pollutes the global namespace. I think Nowide should do what other Boost libraries do, and declare the prototypes in a private detail namespace instead.
>

Ok I need to check this. It something new for me.

>> - What is your evaluation of the documentation?
>
> Incomplete. Too few examples and the reference documentation leaves many things unclear. IMO the reference documentation should unequivocally define the contract of each API.
>
> Also I think the documentation should make it very clear what Nowide does and doesn't do on different platforms.

Yes it was mentioned by several reviewers and will be addressed.

>> And most importantly:
>> - Do you think the library should be accepted as a Boost library? Be sure to
>> say this explicitly so that your other comments don't obscure your overall
>> opinion.
>
> I certainly wouldn't vote against it. I didn't spend enough time to understand every aspect of it though, so I don't feel qualified to vote for it either. I would however suggest to require a major update of the documentation before accepting it.
>
> Regards,
> Paul Groke

Thank You for the review and the valuable comments!

Artyom Beilis