Discussion:
[boost] Review of Nowide
Chris Glover via Boost
2017-06-20 03:44:19 UTC
Permalink
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Artyom Beilis via Boost
2017-06-20 08:00:56 UTC
Permalink
On Tue, Jun 20, 2017 at 6:44 AM, Chris Glover via Boost
Post by Chris Glover via Boost
Hi,
The following is my review of the proposed Boost.Nowide library.
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).
The reason nowide implemented fstream is because filesystem::fstream
uses internally the default std::basic_filebuf thus relays on std library
to provide filebuf::open(wchar_t const *).

Also it works OK for MSVC, MinGW libstdc++ does not provide wchar_t
interface. For filesytem the fix is far from trivial as requires implementation
of filebuf so nowide implemented one.
Post by Chris Glover via Boost
- 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.
I think it is very good point.
Post by Chris Glover via Boost
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.
Ok I hope once I have full test suite of Boost suite it will be easier
to find the stuff.
Post by Chris Glover via Boost
Yes. I am already using it and expect I will be using it a lot more going
forward.
Thank You very much for the review
Post by Chris Glover via Boost
-- chris
Artyom

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Chris Glover via Boost
2017-06-20 11:48:32 UTC
Permalink
Post by Artyom Beilis via Boost
The reason nowide implemented fstream is because filesystem::fstream
uses internally the default std::basic_filebuf thus relays on std library
to provide filebuf::open(wchar_t const *).
Also it works OK for MSVC, MinGW libstdc++ does not provide wchar_t
interface. For filesytem the fix is far from trivial as requires implementation
of filebuf so nowide implemented one.
OK, makes sense.
Post by Artyom Beilis via Boost
Thank You very much for the review
You're very welcome.

-- chris

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Loading...