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
On Tue, Jun 20, 2017 at 6:44 AM, 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.
- 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.
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.
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
-- chris
Artyom
On Tue, 20 Jun 2017 at 04:01 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.
Thank You very much for the review
You're very welcome. -- chris
participants (2)
-
Artyom Beilis
-
Chris Glover