[boost] Review of Nowide

Subject: [boost] Review of Nowide
From: Peter Dimov (lists_at_[hidden])
Date: 2017-06-14 02:02:03

> - What is your evaluation of the design?

The general principles on which the library is build are solid, the design 
is good. As already noted, I would prefer `stat` and `opendir`/`readdir` as 
additions, while we're at it. Yes, we could use Filesystem. On the other 
hand, we _could_ use Filesystem for remove, rename, and fstream, too.

> - What is your evaluation of the implementation?

I didn't look at the implementation too deeply, but I did take a look at, 
and run, the test suite.

There are a number of copy/paste references to Boost.System here 
(test/Jamfile) and Boost.Locale there (index.html), which should ideally be 
corrected. Also, the test Jamfile is missing

    import testing ;

which I do not hold against the author in any way as the Boost.System 
Jamfile also had it missing (it works without it unless invoked from the 
Boost root.)

The doc/ directory is missing a Jamfile, and there's also no 
meta/libraries.json.

There are no .travis.yml and appveyor.yml files in the root, which is odd in 
this day and age. :-)

The global BOOST_USE_WINDOWS_H macro should probably be respected.

> - What is your evaluation of the documentation?

Does the job, although not all functions had their behavior documented in 
the case the passed UTF-8 sequence is invalid.

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

The library, or something isomorphic to it, is indispensable for any serious 
Windows work.

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

As mentioned, I ran the test suite on various versions of msvc (all passed), 
MinGW gcc (warnings in c++03 mode, errors in c++11 and above), and Cygwin 
gcc/clang (errors because of missing ::getenv and friends).

The problem MinGW gcc 7 (and Cygwin clang, once I enabled the Windows path 
in cenv.hpp) identified was

In file included from test\test_system.cpp:11:0:
..\../boost/nowide/cenv.hpp: In function 'int boost::nowide::putenv(char*)':
..\../boost/nowide/cenv.hpp:104:41: warning: comparison between pointer and 
zero character constant [-Wpointer-compare]
         while(*key_end!='=' && key_end!='\0')
                                         ^~~~

which indicates that (1) the library is missing a test for putenv with the 
string not containing '=', and (2) the author should consider testing on 
Cygwin/Mingw, either locally, or on Appveyor. These two may not be important 
production platforms, but they enable testing the Windows-specific code 
paths with g++ and clang++, which as the above shows, may uncover potential 
problems that testing on MSVC alone would miss.

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

Quick reading.

> - Are you knowledgeable about the problem domain?

More or less.

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

Yes. Nowide should be accepted.