Re: [boost] Review of Nowide

Subject: Re: [boost] Review of Nowide
From: Artyom Beilis (artyom.beilis_at_[hidden])
Date: 2017-06-14 05:06:14

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

Ok I'll fix it

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

What it does and why do I need one?

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

Ok.

What I remember from Boost.Locale that doxygen wasn't run during doc build
but rather HTML was pushed to directory. If today there is support of automatic
building of docs with vanilla Doxygen (out of quick book scope) I'll add one.

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

What do I miss?

> The global BOOST_USE_WINDOWS_H macro should probably be respected.
>

Ok good point I'll fix it.

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

Good point.

>> - 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')
>                                         ^~~~
>

Ooops it is serious I'll fix it.

https://github.com/artyom-beilis/nowide/issues/14

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

I general I tested using MinGW and MSVC also never tried clang on
Windows - it is probably time to.

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

Thank You for your effort and valuable inputs.

Artyom Beilis