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

Subject: Re: [boost] [review] Review of Nowide (Unicode)
From: Groke, Paul (paul.groke_at_[hidden])
Date: 2017-06-23 12:11:24

> -----Original Message-----
> From: Boost [mailto:boost-bounces_at_[hidden]] On Behalf Of Artyom
> Beilis via Boost
> Sent: Donnerstag, 22. Juni 2017 22:43
> To: boost_at_[hidden]
> Cc: Artyom Beilis <artyom.beilis_at_[hidden]>
> Subject: Re: [boost] [review] Review of Nowide (Unicode)
> 
> 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.

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 <boost/thread/win32/thread_primitives.hpp>. Pretty straight forward, you'd basically just need to add a namespace around your definitions.