Re: [boost] [AFIO] Formal review

Subject: Re: [boost] [AFIO] Formal review
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2015-09-01 19:54:39

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

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/