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

Subject: Re: [boost] [AFIO] Review (or lack of it)
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2015-08-28 05:31:03

On 28.08.2015 05:28, Niall Douglas wrote:
> On 27 Aug 2015 at 12:54, Roland Bock wrote:
>
>>    * "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.

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.

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

The implementation in Boost should preferably be free from lecacy code. 
If that code is generally useful then why is it not in the library? If 
it's only usefult to you then keep it in your own repo separate from the 
library.

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

Please, don't do that (both print anything and call std::terminate). In 
my projects I'm very often required to remove any output to the console 
and redirect it to a (custom) log library. Third party libraries that 
have hard-coded console output are always a pain and have to be patched.

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.

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