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
> * 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
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))
> 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
> 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.
> Paul Groke
Thank You for the review and the valuable comments!
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk