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-21 00:18:47

Hi,

this is my official review of the Nowide library. I haven't had much time to look at Nowide, but I have noticed some things that might be of interest, so here we go...

> - What is your evaluation of the design?

Overall the design looks good. Some things I'd suggest to reconsider:

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

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

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

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

> - 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. Especially that it doesn't do anything Unicode-related on POSIX platforms and that using it does NOT mean the application will only have to deal with UTF-8 strings on POSIX platforms. Also parts of the documentation that suggest or even _state_ that systems like Linux internally use UTF-8 should be fixed.

> - What is your evaluation of the potential usefulness of the library?

I'd say it's useful.

> - Did you try to use the library? With what compiler? Did you have any
> problems?

No.

> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

Quick reading of the documentation + some very quick glances at parts of the implementation.

> - Are you knowledgeable about the problem domain?

More or less. I have never used or written a "nowide" layer myself, but I have spent plenty of time "widening" Windows-only applications.

> 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