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

Subject: [boost] [AFIO] Review (or lack of it)
From: Roland Bock (rbock_at_[hidden])
Date: 2015-08-27 06:54:31

Hi,

I vote for "No, the library should not be accepted". I do not vote this
way because the library is bad. I do so because I have no way of telling.

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.

b) AFIO depends on other libraries which are probably going to be
proposed for boost, but are not in yet. Regardless of whether there is a
precedent or not, this is far from being ideal. Judging by the
discussion about "Monad", getting that library into boost is not just a
mere formality. Thus, Monad implicitly IS part of the review, because if
it is not accepted (which we don't know today), it will become part of AFIO.

Thus, the scope of the review is not defined correctly.

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.

Of course, boost libraries evolve over time. There will be new versions
of the interface. But starting with that kind of baggage? And asking for
a review that necessarily has to cover everything that is currently
available?

d) The implementation of 1.4 will be replaced completely.

Thus, the implementation (which I consider an important part of the
review), cannot be reviewed in any way that is worth anyone's time.

e) Documentation

  * Please drop the "using namespace XYZ" from all examples. Maybe use
    namespace aliases.
  * The "cheat-sheet" for the impatient is beyond me. I have no idea
    what it is trying to convey.
  * I would certainly prefer ONE page with planned features, not
    sprinkled info here and there (especially not on the cheat sheet).
  * Redundancy: For example, all the dir() functions share the same
    example, don't they? I haven't done a diff and I won't. But it makes
    me nervous to see the same example over and over again. Why not make
    one page explaining all the dir() functions and then use ONE
    example? Also the "Note that ..." text seems to be the same on all
    pages. This piles up to wonderfully large amounts of documentation,
    but I have no intention to reading all of that ever repeating stuff
    trying to find the needle in the haystack that actually contains
    different information. This has to be condensed.
  * There are deprecated functions? Again, I strongly recommend to start
    without the extra baggage.
  * The mix of history and rationale is hard to read, IMO. I'd be
    interested in the rationale, but not in the many steps it took to
    get there.
  * I do not get the world's simplest blob store in the STL form. It is
    most definitely not the simplest blob store. Not even the simplest
    persistent blob store. Why would I need a monad there? Why is there
    a writable flag (why isn't constness enough to prevent writing)? Why
    isn't the directory created at construction once and for all?
    BTW: The name validation fails to check for empty string.
  * 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).
  * "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.

f) Code (since the implementation is going to change anyway, this might
all be useless):

  * /attic: Seriously?
  * I am not a big fan of #if 0 in release code
  * I doubt that std::cerr should be in this library's code.
  * 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.
  * 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.

In the current situation, I cannot do a formal review that would result
in acceptance.

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 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.

Regards,

Roland