Re: [boost] [AFIO] Review (or lack of it)

Subject: Re: [boost] [AFIO] Review (or lack of it)
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2015-08-27 22:28:22

On 27 Aug 2015 at 12:54, Roland Bock wrote:

> a) There is so much going on about personal hardships (e.g. hundreds of
> hours of family time and 12-4am sessions and fried disks and no
> electricity and suspected vendettas and supposedly useless reviews).
> 
> None of this has anything to do with a library review. Why mention it,
> even if you feel it? All of this is bound to make it hard to be impartial.

Well, I don't know what to say to this, so I'll say nothing and leave 
your words speak for themselves.

> c) There are older versions in there. There is 1.3 and 1.4 at least (are
> there more?). Please choose one. Go for it.

AFIO provides strong ABI and API evolution guarantees. It guarantees 
that if you compile and link to v2 of the ABI, your code will 
continue to compile and link no matter what changes next in AFIO.

Some would call that a *huge* feature very atypical of Boost 
libraries and one of exceptional value. You appear to consider it a 
fault.

> e) Documentation
> 
>   * Please drop the "using namespace XYZ" from all examples. Maybe use
>     namespace aliases.

I am unaware of any case where I imported a namespace except into a 
local context. You may have been confused by the very commonly used:

using BOOST_AFIO_V2_NAMESPACE::something;

... which is binding "something" from AFIO into the local or global 
namespace. This pattern is safe.

> Why would I need a monad there?

The original idea was to show how monad abstracts away whether an 
interface is synchronous or asynchronous. It confused people. It was 
a mistake.

> Why is there a writable flag (why isn't constness enough to prevent
> writing)? 

I have no idea what you're going on about here, but constness at the 
language level has nothing to do with whether you open an on-disk 
store for reading or writing.

> Why isn't the directory created at construction once and for all? 

You appear to have not realised that this step enables invariance to 
the store directory being relocated by third party processes during 
operation. This is despite the tutorial text and the comments saying 
this.

>     BTW: The name validation fails to check for empty string.

Logged to https://github.com/BoostGSoC13/boost.afio/issues/99.

>   * https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/workshop/naive_afio_async/naive_racy.html
>     mentions AFIO v1.40 (thats 36 numbers in the future). If the
>     pictures are not loaded, the performance numbers in the table do not
>     say what they are (the per sec is missing).

Logged to https://github.com/BoostGSoC13/boost.afio/issues/100.

>   * "The /final/ named blob store design" starts with "Caution: This
>     section was not finished". 
>   * "The performance of and problems with this third generation design"
>     consists of "This section was not completed in time[...]"
>   * So basically the crucial parts of the blob store sequence are
>     incomplete at best.

The final example is over 1000 lines of code. I didn't think it 
mattered hugely to the review.

> f) Code (since the implementation is going to change anyway, this might
> all be useless):
> 
>   * /attic: Seriously?

AFIO will be three years old soon. It had a life before being ported 
to Boost in 2013. There are still bits and pieces in the attic 
directory of use to me.

>   * I am not a big fan of #if 0 in release code

For the examples, logged at 
https://github.com/BoostGSoC13/boost.afio/issues/93.

In the implementation, if it's #if 0 it's almost always debug assist 
code which I turn on from time to time to help fix some reported bug. 
Or it's code which isn't fully baked yet and end users shouldn't use 
it e.g. byte range locking.

>   * I doubt that std::cerr should be in this library's code.

Use of std::cerr is only when something truly unusual occurs, like we 
are about to fatal exit the process and I would assume the user would 
like to know why we just called std::terminate. Or we are about to 
deadlock, and again the user probably wants to know why their 
application has just hanged itself.

>   * I wonder why there really is a need to have your own path stuff on
>     top of boost::filesystem. If boost::filesystem is insufficient,
>     wouldn't it be better to contribute to that one? Haven't looked too
>     closely, though.

afio::path subclasses boost::filesystem::path or 
std::filesystem::path. It is not a separate filesystem path 
implementation, and in fact it is a very thin wrapper. Its purpose is 
purely to add type safety for NT kernel path conversions from Win32 
paths so the compiler will complain if you try to do something stupid 
like feed a NT kernel path to a filesystem path consuming function.

I did take the opportunity to fix up the deviations in 
boost::filesystem::path from the Filesystem TS however.

>   * I looked briefly into detail/. I would not want to look for a bug
>     there. I have not done any detailed analysis to base this on, but: I
>     would assume that with some refactoring (moving the platform
>     specific parts into platform specific headers), the readability
>     could be increased dramatically. Again: This is just a guess.

The platform specific parts are already in platform specific files. 
afio.ipp is for POSIX which includes MSVCRT. afio_iocp.ipp is for 
Windows IOCP.

> I recommend to treat the current review period as a pre-review and
> 
>   * Do a review of Monad (or whatever it will be named) first, maybe API
>     Bind, too, depending on whether that is going to be used in AFIO
>   * Remove all old versions, everything deprecated and do the
>     implementation change that is planned
>   * Cleanup the documentation. Get it read by people with different
>     knowledge of the problem domain. I am willing to read it as somebody
>     who is interested but certainly not very knowledgeable.
>   * Let the dust settle a bit
>   * Start a new review
>   * Even if you believe in vendettas, stay calm and answer in a
>     matter-of-fact way. My experience with the boost list is, that there
>     are strong voices here which have a very finely tuned sense of fairness.

I do not believe in vendettas, and you will never have seen me pursue 
one anywhere on the internet anywhere in the past decade. Feel free 
to search google if you want to prove this in the nearly 30,000 
public posts I believe I have made by now. I do however respond when 
people are attacking me behind the scenes and the leadership have 
specifically disclaimed doing anything about it (they claim to be 
unaware of anything happening). If I did not defend myself, nobody 
would.

The fact you are aware of any of this is because of my efforts to 
make public the fact this stuff was going on behind the scenes. If I 
had done nothing, nobody here would be the wiser. It is what it is.

> I did invest several hours in this review, reading the mail threads,
> reading the documentation, looking through some code, writing this mail.
> As stated above, I do consider myself interested but not very
> knowledgeable in the problem domain.
> 
> I would like to see a library for asynchronous file IO in boost, but I
> don't think AFIO is ready yet.

Thank you for your review. You caught some points nobody else had, 
including myself.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ 
http://ie.linkedin.com/in/nialldouglas/