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