On Saturday 29 August 2015 01:29:26 Niall Douglas wrote:
On 28 Aug 2015 at 12:31, Andrey Semashev wrote:
On 28.08.2015 05:28, Niall Douglas wrote:
The final example is over 1000 lines of code. I didn't think it mattered hugely to the review.
You mentioned this earlier as well. I keep wondering why an example requires more than 1000 lines of code? Is it because the task is too broad or because the library interface is too verbose? In both cases something should be done about it - either change the docs or the library.
The earlier steps in the tutorial showed a design probably close to what the average C++ programmer might first think of and how you might implement that in STL iostreams and then AFIO. All those steps are complete, and with benchmarks.
The final step shows a design that an expert in the field would write as an example of a real world solution to a real world problem as a comparator to the earlier examples. Similar to lock free memory programming, the final step is entirely file system lock free and uses numerous tricks of the trade to achieve a superb technical solution to the original problem of a simple key value store. It contains memory mapped dense hash maps (alone is about 250 lines of implementation), a concurrent transaction conflict resolution mechanism, resilience to incomplete and/or partial writes and corrupted metadata including power loss, and a complete versioning system letting you replay the history of transactions modifying the store. It achieves late durability without requiring fsync by making use of concurrent atomic appends to enforce extent aging and flushing, so performance is really excellent compared to alternative designs.
The fact all that fits into just 750 lines (1000 - 250 for the dense hash map) is a demonstration of just how much utility and time saving AFIO delivers. But for sure it's going to be gobbledy gook to anyone not versed in the field which is precisely why it both should be in there and isn't important for a Boost review in my opinion.
Do note that the documentation *does* describe the final stage in detail and how it works. I felt that level of detail sufficient for a review, though the comparative benchmarks would have been nice. As I mentioned, I had a hard drive failure which got in the way. It happens.
All that sounds great but regardless of its value an example of 1000 lines of code just isn't comprehensible. You mentioned several design points that compose this example (hash maps, transactions and so on), which means the example is decomposable. Perhaps you should document these bits separately along with any specifics and benefits wrt Boost.AFIO. Maybe even include tools for implementing these things into Boost.AFIO. But don't unload a heavy pile of code on the reader.
Calling std::terminate is also something I would call unexpected and harmful. You don't know what the application is doing - one thread may be doing something with Boost.AFIO and another may be doing domething important and unrelated. Throw exceptions on errors, where possible. Document UB when not.
If AFIO is fatal exiting the process, you are in an unrecoverable cannot continue situation. I mainly have those traps in there for handling memory corruption which is not uncommon when writing file system applications. They are *extremely* useful for catching inadvertent memory corruption.
And they should never trip unless memory corruption has occurred or you have a serious error in how you are using AFIO.
What kind of memory corruption do you mean and how do you detect it? Also how terminating the process helps in fixing those errors?
Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
Let it deadlock. Seriously. At least the user will be able to see it blocked and get a backtrace to figure out how did it come to this. Terminating the app is the most useless and harmful reaction.
Fatal exit happens when AFIO cannot continue without losing or corrupting other people's data. I make no apology for that - it is the right thing to do.
Why is this even possible with the library? And why not report an error in a usual way?
Deadlocks are handled by continuing to deadlock in debug builds, though with plenty of help and backtraces etc printed to std::cerr. This lets you interrupt in a debugger and try and figure out the cause. In release builds, after a reasonably long timeout you get a terse single line message to std::cerr and it breaks the deadlock and mops up the orphaned state as best it can before continuing.
In production there are release builds, not debug. We don't even use debug builds while testing because they don't really add anything but the possibility for the error to slip unnoticed in release builds. You may say this is an unusual policy but an least at two jobs I worked at practiced this. Altering the state before exiting is also something that doesn't look right to me. Having the latest state of the crashed process is sometimes helpful in debugging. Along with console output and unexpected program termination this is one of the "never do that" things. I think you're trying to be too smart in your library. Don't. You will never cover everything users want. There will always be someone like me who wants a different behavior than you envisioned.