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

Subject: Re: [boost] [AFIO] Review (or lack of it)
From: Roland Bock (rbock_at_[hidden])
Date: 2015-08-28 02:29:33

On 2015-08-28 04:28, Niall Douglas wrote:
> On 27 Aug 2015 at 12:54, Roland Bock wrote:
>
>
>> 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.
ABI/API evolution guarantees are cool, great, wonderful, nice, huge. Yes.

Still, it would make it easier to review the library if the scope was
limited to 1.4. Having two API versions to start with doubles the effort
for analyzing the API. It also increases the amount of code that has to
be maintained in your boost library.

That is an issue. You make harder for yourself and every reviewer this way.

>
>> 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.
Copied from the introduction page:

using namespace BOOST_AFIO_V2_NAMESPACE;

>  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.
I am not a huge fan of that either if it happens in the first few lines
of a long example.

It is not about safety, it is about comprehensibility. On the
introduction page, you have:

using namespace BOOST_AFIO_V2_NAMESPACE;
using BOOST_AFIO_V2_NAMESPACE::rmdir;
...
auto rmdir(async_rmdir(depends(barrier.front(), mkdir)));
// Check ops for errors
boost::afio::when_all_p(mkdir, mkfile, mklink, rmlink, rmfile, rmdir).wait();

The first line is bad, the second is confusing at best (IMHO).

>> 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.
First of all, please reread your claim: "World's simplest named blob
store in STL iostreams"

What you present is definitely not what you promise.

The simplest store would

  * create the folder to store stuff in at the beginning (you could use
    a flag here, but we are talking about the /simplest/)
  * you can always attempt to read (const operation)
      o For reading, you would never open a file for writing on the disk
  * your store has to be non-const if you want to write (non-const
    operation)
      o For writing you would always open a file for writing on the disk

Turns out that language level constness expresses exactly the same as
the read/write access on disk after construction. It is also much more
friendly towards the user of the store, since the compiler will catch
invalid write attempts.

>
>> 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.
No, you appear to be under the impression that the simplest thing has to
be pretty complex already and react in ways that might or might not be a
good strategy for my use case.

If somebody moves the store away, while I am working on it, do I really
want it to start over (as if nothing happened)? The simplest store will
merely say "did not find" if you want to read and "could not write" if
you want to write.

If I use the simplest store in the world, I have to take care of third
parties doing nonsense to my store.

You even mention that in the conditions: No third party must mess with
the store. So why do you treat that case then?

>
>>     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.
Well, it is the culmination of the previous chapters. The place where
you bring it all together and present the results of those efforts. How
can that not matter?

>
>> 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.
Since the boost port is under review, I suggest getting rid of such
stuff. It distracts from the code you want to be reviewed. Seeing an
obvious attic, I wonder which other parts of the code might be out of
use? Would I be looking at the right stuff when I look into the code?

You are using git. git can store the attic. It won't get lost. But
reviewers should not see it. That's all I am saying.
>
>>   * 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.
Why don't you replace

#if 0

with

#if BOOST_AFIO_USE_HALF_BAKED

 or similar?

Also, git has branches. Why don't you work on the half-baked stuff in a
feature branch?

#if 0 is code smell.

>
>>   * 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.
Quite a few applications turn off or ignore std::cerr.
At least given users the option to provide their own reporting function
for emergencies.

>>   * 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.
Hmm. The code I looked at was still riddled with #if THIS_SYSTEM do that
#else do something else.

>
>> 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,
You claimed that you were victim of one more than once in the context of
the AFIO review. That's why I mentioned it.

> If I did not defend myself, nobody 
> would.
Based on what I see on this list, I doubt that. But there is no way of
proving it one way or another.
>> 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.
>
Glad to help.

And with the points where we don't see eye to eye: I am happy to explain
if we meet at CppCon.

Best,

Roland