1. Should Boost.AFIO be accepted into Boost?
This library must be in Boost, but not in it's current state. Some polishing required and some improvements. Dependent libraries must be accepted in Boost/dropped/documented and reviewed as part of AFIO. After that another review must be done. So it's NO for now.
2. What is your evaluation of the design?
NONBACTH operations seem right and good. Some simplification must be applied, by possibly removing some of the overloads and reducing amount of types visible to user (`handle_ptr`, `*_req`). BATCH operations do not seem right. They must be removed or their interface must be greatly reworked. afio::path must be hidden from user or filesystem::path must be used (I'm not 100% sure, but it looks like `\\?\` paths are OK to be in filesystem::path http://www.boost.org/doc/libs/1_59_0/libs/filesystem/doc/reference.html#long... ). More functions for everyday use may be provided (async_copy for example)
3. What is your evaluation of the implementation?
Header files must be split into multiple smaller ones. Multiple memory allocations must be removed from library internals. Implementation must be polished. Recommendation: ABI stable part of AFIO may be moved in a separate git branch to untie the AFIO library developer's hands. In that case new version of AFIO may break the ABI without affecting users and older versions could be removed from sources.
4. What is your evaluation of the documentation?
That's one of the biggest problems of the library. Documentation is not structured and sometimes overcomplicated. Some notes for simplification: * "AFIO single page cheat sheet for the very impatient" -> "Getting started". Move table to the top of the page, replace the "Related types" column with "Boost.Filesystem sync alternative", replace numbers with checks/+/-. * Move the "Example mini-programs written using AFIO" chapter right after the "Getting started", rename chapter into "Tutorial", refactor second example to not use batch operations and to use only free standing functions. Third and fourth examples must be greatly simplified and probably split into smaller examples. * Move the content of "Step by step workshop building an AFIO-based key-value store" into the "Tutorial" after the previous examples, rename it into "Key-value store walkthrough", simplify and make shorter if possible * "Compilation" section must be somewhere around "Getting started" * Other docs must be after the "Getting Started," "Tutorial" and "Compilation" * "Reference" section must be refactored. All the overloads must be probably described and showed on a single page (all the overloads, including those that explicitly accept dispatcher) * Shorten and simplify the examples wherever possible.
5. What is your evaluation of the potential usefulness of the library?
Very very useful.
6. Did you try to use the library?
No.
7. How much effort did you put into your evaluation?
About 20 hours.
8. Are you knowledgeable about the problem domain?
I have a fair amount of experience with asynchronous programming in C++ and I have pretty extensive experience with designing interfaces for async operations. Have not dealt with async fiesystem operations before. -- Best regards, Antony Polukhin
On 31 Aug 2015 at 22:40, Antony Polukhin wrote:
afio::path must be hidden from user or filesystem::path must be used (I'm not 100% sure, but it looks like `\\?\` paths are OK to be in filesystem::path http://www.boost.org/doc/libs/1_59_0/libs/filesystem/doc/reference.html#long... ).
As I mentioned in another thread, afio::path isn't for input control, it's to make sure NT kernel paths output by AFIO are not accidentally fed to other C++ code accepting filesystem::path without being converted to a Win32 path first. It is not a one to one conversion from NT kernel to Win32 path - there are two conversion API choices with at least four possible Win32 path outputs, and tradeoffs in which you choose. You are very much not guaranteed that the Win32 path you send in will have any resemblence to the Win32 path which comes out after conversion (blame Windows for this, there is a lot of Win16/32 legacy baggage in there). I cannot use \??\ prefixes because when I ask for the canonical current path of an open handle (needed for race protection), I get back the true NT kernel path with any kernel symlinking (such as ??, junction points etc) removed. NT is also quite slow at parsing paths with redirects in them, so using the canonical true path makes any operation doing path related work noticeably faster. You should find AFIO is about 50% faster to open a file than the Win32 API, and gets even faster the longer the path. If you did not have file system race free support, then direct use of filesystem::path should be unproblematic (it was in AFIO up to v1.2). One could support *both* approaches I suppose depending on configuration, but that broadens my corner case testing surface enormously and I am unconvinced that that fits into AFIO's goals which is write once run anywhere as far as is possible. I think an async filesystem library without race free file system is probably not AFIO.
3. What is your evaluation of the implementation?
Header files must be split into multiple smaller ones. Multiple memory allocations must be removed from library internals. Implementation must be polished.
All agreed and already logged.
Recommendation: ABI stable part of AFIO may be moved in a separate git branch to untie the AFIO library developer's hands. In that case new version of AFIO may break the ABI without affecting users and older versions could be removed from sources.
APIBind already implements this in the presented library.
4. What is your evaluation of the documentation?
That's one of the biggest problems of the library. Documentation is not structured and sometimes overcomplicated. Some notes for simplification: * "AFIO single page cheat sheet for the very impatient" -> "Getting started". Move table to the top of the page, replace the "Related types" column with "Boost.Filesystem sync alternative", replace numbers with checks/+/-. * Move the "Example mini-programs written using AFIO" chapter right after the "Getting started", rename chapter into "Tutorial", refactor second example to not use batch operations and to use only free standing functions. Third and fourth examples must be greatly simplified and probably split into smaller examples. * Move the content of "Step by step workshop building an AFIO-based key-value store" into the "Tutorial" after the previous examples, rename it into "Key-value store walkthrough", simplify and make shorter if possible * "Compilation" section must be somewhere around "Getting started" * Other docs must be after the "Getting Started," "Tutorial" and "Compilation" * "Reference" section must be refactored. All the overloads must be probably described and showed on a single page (all the overloads, including those that explicitly accept dispatcher) * Shorten and simplify the examples wherever possible.
Logged as https://github.com/BoostGSoC13/boost.afio/issues/114. Thanks for the review Antony. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
participants (2)
-
Antony Polukhin -
Niall Douglas