[boost] Review of Nowide

Subject: [boost] Review of Nowide
From: Chris Glover (c.d.glover_at_[hidden])
Date: 2017-06-20 03:44:19

Hi,

The following is my review of the proposed Boost.Nowide library. This is my
first official review of a boost library, so please forgive me if I've
missed anything.

- What is your evaluation of the design?

At first I was surprised by lack of a platform agnostic encoding. However,
through reading the docs and the discussion in the list, I believe this to
be the correct choice. If a user has a need for a common encoding, I
believe that would be best served by a different library that would
ultimately have different issues and performance characteristics.

One thing I'm not sure about is if the overlap between the fstream
implementations in filesystem and nowide is necessary. It seems to me that
if I set filesystem to be nowide aware (via
boost::nowide::nowide_filesystem) then there's little reason for the nowide
versions of the stream objects to exist, other than possibly eliminating
one string copy on non-windows platforms and avoiding the filesystem
dependency (and maybe those are good enough reasons).

- What is your evaluation of the implementation?

I only gave the implementation a glance. It looks good; clean, organized;
with a full test suite and nothing weird stands out. It seems to be of the
typical quality I expect from a boost library.

- What is your evaluation of the documentation?

The documentation is clear and complete, but I think it needs to explain at
the top that the library does not present a uniform encoding for strings.
This is discussed in the documentation near the end, under "Q & A", but I
think it should be near the top. A suggestion is to add a section called
"What Boost.Nowide is *not*" directly under the section titled "What is
Boost.Nowide", otherwise I expect some users to be surprised.

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

It is very useful. I am already using it.

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

I converted one application that was using boost::filesystem::path to
transport filenames around to use nowide instead. I had no issues,
everything just worked. Some code was also simplified as a result -- not
dramatically, but enough that it felt better.

I tested the application on windows only using Visual Studio 2017.

I also ran the tests under MSYS2 on Windows using gcc-6.3, on Windows using
MSVC 2017 and on Ubuntu using gcc-7.

Under MSYS, one test failed (Fail: Error boost::nowide::cin.putback(c) in
test_iostream.cpp:17 main), but I haven't had a chance to look into why
yet.

All of the tests passed on the other platforms.

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

I followed the discussion, read the docs, integrated the library, used the
API and tested the resulting program. All in, I think that took me about 4
hours. This seems quick, but to me that is a testament to a solid design
and clear documentation because the conversion process was very simple.

- Are you knowledgeable about the problem domain?

I am aware of the problem and have worked around it in the past by using
filesystem::path as a string. However, most of the environments I work in
professionally are controlled in a way that allows us to avoid this problem
most of the time, so I have not had extensive experience with it.

- Do you think the library should be accepted as a Boost library?

Yes. I am already using it and expect I will be using it a lot more going
forward.

-- chris