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

-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Artyom Beilis via Boost Sent: Donnerstag, 22. Juni 2017 22:43 To: boost@lists.boost.org Cc: Artyom Beilis Subject: Re: [boost] [review] Review of Nowide (Unicode)

On Wed, Jun 21, 2017 at 3:18 AM, Groke, Paul via Boost 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.

They will fail if the user-provided buffer isn't big enough.

...

* 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.

OK, in that case it should be fine.

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)

OK. How about mixing use of the boost::nowide:: and the std:: versions? If that possible? Because that's something that I guess will be hard to avoid.

...

* 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,

I don't see how they are "mostly internal". Every application that needs to do some Windows-specific things will require them. If it's internal it should go into a detail namespace and not be documented in the generated documentation (doc in the source is fine of course). Otherwise it's a public API and IMO deserves consideration.

...

...

- 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.

Cool. You can find an example in . Pretty straight forward, you'd basically just need to add a namespace around your definitions.