dinit-iostream: Custom new special-purpose library

[mobin-2008]

This is replacement for C++ standard library iostream/fstream.

See dinit-iostream.h for more information and the usage.

Closes #240

Signed-off-by: Mobin "Hojjat" Aydinfar <mobin@mobintestserver.ir>

[davmac314]

I reviewed mostly one file (dinit-iostream.h) and I have a lot of comments so I will stop here and let you address them before adding more.

A few general comments:

• it seems to me like there's more in common in istream and ostream that could be moved into streambuf_base (and perhaps streambuf_base and fstream_base should be merged)
• buffer handling seems to be suboptimal, for example the buffer isn't necessarily completely full before it is flushed. Eg. suppose I write 16380 bytes followed by 10 bytes followed by 512 bytes and then flush. That should need only two writes (buffer size is 16384) but it will do three.
• "move" constructor doesn't make sense, I'm still waiting for explanation of why it's needed, and even if it is, why doesn't it transfer buffer ownership?
• trying to write again after write failed previously is not a good idea, and trying to do a write of later data after earlier flush failed is very wrong - in the worst case it will skip the data from the buffer and still write the later data, so a chunk of written data will just be missing from the output.

[davmac314]

• buffer handling seems to be suboptimal, for example the buffer isn't necessarily completely full before it is flushed. Eg. suppose I write 16380 bytes followed by 10 bytes followed by 512 bytes and then flush. That should need only two writes (buffer size is 16384) but it will do three.

Sorry, this example might not be right. Consider instead: 16380 bytes followed by 8 bytes followed by 16380 bytes again.

[davmac314]

@mobin-2008 you made a comment here but it seems to have been deleted. Let me know when the PR is ready for review.

[mobin-2008]

Ready for review.

Changelog from your last review:

1. MAX_TYPE_NUMDIGITS() algorithm has been changed to recursive integer logarithm like function.
2. buf->reset() has been removed (because make no difference)
3. Documention has been reworded in terms of vocabulary and grammar.
4. A check has been added to fstream_base::open() functions to disallow null pointer/empty paths.
5. A check has been added to put() function to avoid undefined behavior in strlen() of null string and Segmentation fault on appending null string into buffer.
6. Exception that get thrown by iostream library has been specialized.

[davmac314]

Seems like you've made some changes unrelated to previous review, but haven't addressed all the issues raised in the previous review.

Please make sure to address all issues, and avoid making new changes that weren't discussed unless they solve important problems. Once issues from last review are resolved I will review again.

[davmac314]

I will try to fully explain each issue, so hopefully there's no confusion:

Constructors setting buffer_failbit

The constructors should be properly documented (i.e. document that buffer allocation is done when the constructor is called, but may fail, leaving buffer_failbit set). I see now that you did improve the behaviour, but I didn't see that before because you didn't list it as one of the changes and you included many changes so there was a lot of noise.

(That is one reason why you should not add additional changes when responding to a review. It complicates things, and makes it harder to check that the issues bought up in the review last time were resolved properly).

streambuf set_buf/set_buf_with_owner function

streambuf function set_buf (now called "set_buf_with_owner") is dangerous because it removes the previous buffer even though it may have data in it, without flushing it first. I don't understand the use case for it (i.e. why it's needed) so I don't know whether the best answer is just to have set_buf (... with_owner) flush the buffer, or even just document that you must flush the (current) buffer before calling set_buf; if there is no use case then it should really just be removed (we don't want extra code that isn't used anyway).

i.e. the solution should be one of the following:

• remove it
• make it protected instead of public (if it only needs to be called by subclasses)
• make it flush the old buffer
• document that the old buffer should be flushed before calling

If the solution is any option other than "remove it", you should be able to explain why.

exceptions function

The exceptions function should behave consistently, it doesn't make sense to have a magic value (0) mean one thing and any other value mean something different. If the function does two different things it should be two different functions.

But: I also really don't see why toggling exception bits is useful, why can't I just set the bits I want set?

Example: if I write a function which takes an istream parameter (by reference) how can I ensure the behaviour (exceptions vs non)? Eg how could I ensure throw_on_buffer_fail is set for example?

void read_some_data(istream &instream) {
    // how can I turn on buffer_failbit in exceptions?

    // If I do:
    instream.exceptions(buffer_failbit);
    // Now I don't know whether buffer failure throws exceptions or not!
}

[mobin-2008]

I will try to fully explain each issue, so hopefully there's no confusion:

Constructors setting buffer_failbit

The constructors should be properly documented (i.e. document that buffer allocation is done when the constructor is called, but may fail, leaving buffer_failbit set). I see now that you did improve the behaviour, but I didn't see that before because you didn't list it as one of the changes and you included many changes so there was a lot of noise.

(That is one reason why you should not add additional changes when responding to a review. It complicates things, and makes it harder to check that the issues bought up in the review last time were resolved properly).

construction examples now include a check to catching buffer allocation failures.

Also I wrote that message to notice there is some other changes with fixing all previous reviews but looks like it made into confusion.

streambuf set_buf/set_buf_with_owner function

streambuf function set_buf (now called "set_buf_with_owner") is dangerous because it removes the previous buffer even
...

i.e. the solution should be one of the following:

• remove it
  ...

getting raw pointer through get_buf() is enough and I removed them.

exceptions function

The exceptions function should behave consistently, it doesn't make sense to have a magic value (0) mean one thing and any other value mean something different. If the function does two different things it should be two different functions.

But: I also really don't see why toggling exception bits is useful, why can't I just set the bits I want set?

Example: if I write a function which takes an istream parameter (by reference) how can I ensure the behaviour (exceptions vs non)? Eg how could I ensure throw_on_buffer_fail is set for example?

void read_some_data(istream &instream) {
    // how can I turn on buffer_failbit in exceptions?

    // If I do:
    instream.exceptions(buffer_failbit);
    // Now I don't know whether buffer failure throws exceptions or not!
}

exceptions() is renamed to throw_on_states() for better meaning. Also it accepts a boolean to throw or not throw on requested bits instead of toggle behavior:

...
// Caller can be sure about that this call will mark eofbit for throwing `dio::iostream_eof` exception for example
instream.throw_on_states(diostates::eofbit, true);
...

Also I added a new function called clear_throws() which disables all situation can throw exceptions instead of magic 0 parameter.
I think it's enough.

[davmac314]

I think it's enough.

Ok, but I've seen more pushes come through since. So I will wait until you request review.

[davmac314]

It is really getting a lot better now.

I found a few logic errors / bugs, and there are still some style issues.

[davmac314]

Mostly now I have suggested changes for the comments but there is one important thing:

write_nx() must return false if it cannot write (or buffer) everything that was supposed to be written.

Likewise write() should throw an exception if it cannot write (or buffer) everything that is supposed to be written.

Otherwise, there is no way to tell that an error occurred.

I thought this is what I had mentioned already but I can't find the comment now. Sorry if I said the opposite by mistake, but it is definitely necessary that write() must write the whole message or (if it can't) return an error.

It's not ok if an error cannot be noticed.

write_buf() is a different case, that can write a partial message and return however much it succeeded in writing (or buffering). The caller knows the buffer length so can detect a partial write and deal with it.

[davmac314]

Getting close now :)

[davmac314]

Remove "same as std::endl"

[davmac314]

Remove "same as std::flush"

[davmac314]

Is it an error to call open(...) if the stream is already open?

So it's allowed? (you didn't answer!)

If it's allowed, what happens to the buffer (does it get flushed or cleared?) and the error states?

If something is allowed, the behaviour should be sensible (and documented).
If it is not allowed, that should be documented ("Must not be called if the stream is already open.")

[mobin-2008]

So it's allowed? (you didn't answer!)

If it's allowed, what happens to the buffer (does it get flushed or cleared?) and the error states?

If something is allowed, the behaviour should be sensible (and documented).
If it is not allowed, that should be documented ("Must not be called if the stream is already open.")

I think this note is good enough:

    // Note: Opening an already opened stream is not allowed and caller must close the stream
    // before open it again.

[davmac314]

Looks like you changed some logic in ostream::put when pushing 4922d35

It looks like it fixes a potential problem where partial writes would result in an error (from write/write_nx), so that's ok, but please make an explanatory comment (on the PR) any time you make changes to code in any PR outside what we've already discussed in the review.

This is getting very close to being acceptable but there are still some issues that I've highlighted.

[davmac314]

Just a few more things.

I don't think I will include this in the 1.0 release, there's not enough time to fully test the changes (and it's still not optimal for handling non-blocking I/O). But we can finalise the PR and have it ready to merge with just these fixes.

[mobin-2008]

CI failure is unrelated.