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

Subject: Re: [boost] [AFIO] Review (or lack of it)
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2015-08-28 19:07:58

On 28 Aug 2015 at 8:29, Roland Bock wrote:

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

That isn't a global using namespace. It could be a local using 
namespace, and in its original source file it is indeed a local using 
namespace.

But fair enough it could be interpreted differently, so logged to 
https://github.com/BoostGSoC13/boost.afio/issues/102.

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

By simplest it was meant the public interface API. Not the 
implementation. I thought that obvious, but logged anyway to 
https://github.com/BoostGSoC13/boost.afio/issues/103.

> Also, git has branches. Why don't you work on the half-baked stuff in a
> feature branch?
> 
> #if 0 is code smell.

These are entirely personal opinion based on suppositions and 
assumptions with zero technical relevance to the quality of a 
library, as was a lot of the stuff I snipped. I don't work in 
pristine code environments, I like to keep around bits of code in #if 
0 blocks to remind me of things, or for the occasional use, or for 
any other reason. I personally would find such segments educational 
when reviewing a library rather than the unfounded claim of "code 
smell" which may be true in other code bases in your experience, but 
I can assure you is not the case in my code bases.

Instead of assuming code smells or other non-factually based 
suppositions, consider examining the coverage of the unit test suite 
and the rigorous testing regime run each and every commit and taking 
some hours to complete.

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

It is trivial to redirect std::cerr elsewhere.

I am happy for std::cerr to become a macro, and this is logged at 
https://github.com/BoostGSoC13/boost.afio/issues/104.

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

Sure. There are substantial deviations between POSIX implementations, 
but not enough to warrant separate files.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ 
http://ie.linkedin.com/in/nialldouglas/