Replace use of iostream-based classes #240
Comments
mobin-2008
added
Enhancement/New Feature
|
Do you mean we need to use Lines 20 to 26 in a6cff08 to this? // Read and set environment variables from a file. May throw std::bad_alloc, std::system_error.
void read_env_file(const char *env_file_path, bool log_warnings, environment &env)
{
int env_file = open(env_file_path, O_RDONLY);
if (env_file == -1) {
return
}
//env_file.exceptions(std::ios::badbit); |
|
No, not necessarily, I meant replace use of |
|
I worked on it a bit and I really enjoy, @davmac314 Please assign this issue to me if you wish. for example I have a very basic implantation for replacing #ifndef DINIT_IOSTREAM_H
#define DINIT_IOSTREAM_H
#include <baseproc-sys.h>
#include <cstring>
#include <cmath>
// ToDo: Desc
namespace dio {
class ostream
{
public:
ostream operator<<(const char *msg)
{
bp_sys::write(1, msg, strlen(msg));
// ToDo: Error handling
return *this;
}
ostream operator<<(std::string msg)
{
bp_sys::write(1, msg.c_str(), msg.size());
return *this;
}
ostream operator<<(int num)
{
char tmp[(int)((ceil(log10(num))+1)*sizeof(char))];
sprintf(tmp, "%d", num);
write(1, tmp, strlen(tmp));
return *this;
}
};
ostream cout;
// ToDo: cerr should print to fd 2 (stderr)
ostream cerr;
}
#endifand |
|
the shift operator overloads are a bunch of gross 90s cargo cult, so while replacing them i'd suggest actually making up reasonable interfaces low level file descriptor funcs are too primitive for reasonable i/o though, so the implementation may get relatively complex |
With something like |
|
Input is the more important issue, and more difficult. For output, fmt is probably overkill. Look at Yes, there should be buffering. Particularly for input but ideally for output also. There are a lot of subtle issues involved in getting this right, so please think carefully, and consider the requirements of the project, existing style and practice, etc. Avoid memory allocation where practical. Throw exceptions appropriately. Provide quality code documentation (comments). |
|
Note that your sample implementation already contains things I would outright reject:
|
|
After a while, I'm so happy to announce that Feel free to take a look at it: Best regards |
|
@mobin-2008 I had a quick look. It's an ok start but I do have lots of concerns and comments. Some stylistic things:
Design-related:
There's probably more but that is at already a lot of things to think about. It's all very clearly based on the
Note that these are genuine questions, I'm not saying the answer is "yes" or "no" in any case, but they should be considered. I'm not sure if you've thought about them. If you have, that's fine, but perhaps you should document your rationale as part of the documentation. Finally: lots of documentation (comments) is needed! Right now so many things are unclear. It shouldn't be necessary to look at the implementation to understand how it should be used. |
|
Most of style things are fixed.
I disagree. /*
* All calls to write() will be buffered but not necessarily written to file descriptor except
* on these situations:
*
* 1. Message has \n (newline)
* 2. "flush()" (or its exception-free variant: flush_nothrow()) be called
* 3. "flush" object be passed to write() (equals to calling flush())
* 4. "endl" object be passed to write() (equals to writing \n and calling flush())
* 5. Object be destructed (which make a call to flush_nothrow())
* 6. Buffer be full
*/Which is OK in my opinion.
Hmm, I'm not so happy with returning error conditions in function because it makes into mess. I think a class should maintain its status. @davmac314 Question: It's safe to assume that Current Details
/* ostream
* =-=-=-=
*
* This class is designed to provide basic functionality around the output. It maintains
* a cpbuffer (with a fixed size which can be changed in compile-time) and stores file descriptor number
* in its private space. it supports empty constructor and constructor with file descriptor
* as parameter:
*
* dio::ostream output_a; // Simple construction of ostream
* dio::ostream output_b(3); // Construction of ostream with file descriptor No.
*
* Also setting (or changing) file descriptor after construction is available by setfd()
* function.
*
* copy/move constructors/assignments and destructor
* -------------------------------------------------
*
* ostream copy constructor is prohibited (copy constructor is deleted).
*
* ostream move constructor is available. All variable contents except of the buffer
* (and its contents), will be moved. Also buffer of moved object will be unavailable
* (by reset() call).
* Class definiton has full list of what will be moved from given object.
*
* ostream copy assignment is same as copy constructor.
* ostream move assignment is same as move constructor.
*
* ostream desctructor will call flush_nothrow() on desctructing.
*
* Public member: write()
* -----------------------
*
* ostream provides write() functions, which support many types of data for writing into fd(s).
* See class definition for full list of types which supported by write() functions.
* It's possible to combine different types in a single call with "," charactor also:
*
* output_a.write("This is a example message.\n");
* output_b.write("2 + 2 equals to: ", 2 + 2, endl);
*
* All calls to write() will be buffered but not necessarily written to file descriptor except
* on these situations:
*
* 1. Message has \n (newline)
* 2. "flush()" (or its exception-free variant: flush_nothrow()) be called
* 3. "flush" object be passed to write() (equals to calling flush())
* 4. "endl" object be passed to write() (equals to writing \n and calling flush())
* 5. Object be destructed (which make a call to flush_nothrow())
* 6. Buffer be full
*
* write() functions guarantee to return number of actually written charactors not number
* of buffered charactors. Also they can return -1 on failure cases (currently badbit).
*
* Public member: flush()
* ----------------------
*
* ostream provides flush() function to write all contents of buffer. Passing flush object
* to write() functions equals to call flush() on the stream. This function will return
* number all written charactors or -1 on failure cases (currently badbit)
*
* Error handling and Stream states
* --------------------------------
*
* ostream states are: goodbit and badbit.
* goodbit is true by default and means "Everything seems to be normal".
* badbit is 0 by default and means there is something wrong from POSIX I/O calls. It's get
* set to POSIX errno in this case.
*
* For checking current stream states these functions are available:
* rdstate(): Returns diostates::goodbit or diostates::badbit based on current state bits.
* good(): Returns 1 or 0 based on goodbit is set or not.
* bad(): Returns 0 or POSIX errno based on badbit value.
*
* For setting current stream state(s) these functions are available:
* clear(): Sets badbit to 0 and goodbit to true.
* setstate(): Set requested bit to requested value.
*
* NOTE: goodbit cannot be set directly to false by hand! goodbit has conflict with other bits
* and setting other bits (e.g. badbit) true will make goodbit false.
*
* Exception handling
* ------------------
*
* exceptions() can be used to set when should a exception be thrown. For ostream it accepts
* diostates::badbit. Also exceptions(0) will disable all cases which can throw exceptions.
*
* See description for each function to know what exceptions can be thrown by functions.
*
* All of exception-throwing functions have exception-free variants which marked by
* "_nothrow" suffix. Those functions guarantee not to throw any exceptions regardless of
* the exceptions() setting.
*/ |
Ok, I see I misread it. But:
Why flush just because of newline? And:
Why does it call flush() if writing '\n' already does flush?
The stream library doesn't have sufficient context to make a decision about how to handle these errors, so it should report them to the application. It should not try the operation again, it's for the application to decide whether that is the right thing to do. It might have made sense to have a mode which ignores and retries on |
|
@mobin-2008 I will have a proper look over what you have when I get a chance (see comment above too; I think flush-on-newline probably isn't the right choice). |
|
Sounds good, I'm going to clear some codes and documention in 30 minutes. |
|
@davmac314 I worked on it a bit and here's the list for pending things:
I try to complete these things |
Ok - please open a PR when you are ready. I had a quick look and the major concerns are gone, but I will still have a lot of comments/questions. |
|
@davmac314 General design question: I really can't find any meaningful usage for copy/move constructor/assignment in all clases. Do you know a meaningful usage of those in |
For For your current design which has stream classes that are "heavier" in the sense that they include the buffer inside the object (which suggests to me that you really don't want to have multiple stream objects to deal with one underlying file/stream), moving between streams probably makes even less sense (especially because it should be possible to get the underlying file handle anyway). |
|
I reworked
Hmm, But it is inefficient in memory because all of the new created objects will have a int main() {
std::string str;
dio::ifstream i2;
{
dio::ifstream i1("file");
i1.open();
if (!i1) return -1;
i1.getline(str, '\n');
i2 = std::move(i1);
}
i2.getline(str, '\n');
return 0;
}Debugger trace of (lldb) print i2
(dio::ifstream) {
dio::istream = {
mainbuf = {
cpbuffer<16384> = (buf = ""..., cur_idx = 0, length = 0)
}
buf = 0x00007fffffff6738
fd = 3
goodbit = true
eofbit = false
failbit = false
badbit = 0
eofreached = true
throw_on_eofbit = false
throw_on_failbit = false
throw_on_badbit = false
}
path = 0x00005555555556e4 "file"
are_we_open = true
}
(lldb) print i2.buf
(dio::streambuf *) 0x00007fffffff6738
(lldb) print *i2.buf
(dio::streambuf) {
cpbuffer<16384> = (buf = "**********---***********\n*******- _ _ -********\n******- 0 0 -*******\n******- | -*******\n******- ___ -*******\n*******- -********\n**********---***********\n***********-************\n***********-************\n**********---***********\n*********-*-*-**********\n********-**-**-*********\n*******-***-***-********\n******-****-****-*******\n***********-************\n***********-************\n**********-*-***********\n*********-***-**********\n********-*****-*********\n*******-*******-********\n******-*********-*******\n"..., cur_idx = 50, length = 475)
}
Documention should advise that using std::move due to current design is memory inefficient and assigning object pointer to a @davmac314 WDYT? |
|
I am confused by your debugger output, it doesn't seem to match the current code. I guess you didn't push the updated code? It looks like you have added a |
|
(The reason |
|
I pushed a new version and it's mostly about buffer's memory allocation. /* streambuf_mgr
* =-=-=-=-=-=-=
*
* streambuf_mgr is buffer manager which provides functions about
* allocating and deallocating streambuf buffers.
*
* All of streambuf_mgr functionaliy and interfaces are exception-free.
*
* copy/move constructors/assignments and destructor
* -------------------------------------------------
*
* streambuf_mgr copy constructor is prohibited (copy constructor is deleted).
* streambuf_mgr copy assignment is same as copy constructor.
* streambuf_mgr move constructor is prohibited (move constructor is deleted).
* streambuf_mgr move assignment is same as move constructor.
*
* streambuf_mgr destructor will deallocate all of the registered buffers.
*
* Public member: allocate_for()
* -----------------------------
*
* allocate_for() is a function to allocate a streambuf and will return that streambuf
* pointer. allocate_for will register all buffers in a std::list (and probably through
* heap allocation) but the streambuf itself will be on stack.
*
* Public member: deallocate()
* ---------------------------
*
* deallocate() is a function to deallocate an streambuf. This function will earse the allocated buffer
* from allocated_buffers std::list.
*
* Public member: setowner()
* -------------------------
*
* setowner() is a function to set the allocated buffer is for which object. this function will find allocated
* buffer and change its "allocated_for" value to passed void pointer.
*/
...
// streambuf_allocator is designed to be responsible for all of buffer allocation
// in the whole program and you probably don't want to create another object
// from streambuf_mgr.
extern streambuf_mgr streambuf_allocator;I think it's most memory efficient solution that we have. |
|
Also If it's ok, I will fix |
|
@davmac314 I think the implementation is ready and just some documentation is missing (because I'm waiting to they get stabilized). |
|
Sorry for spam :( Last major change: /* All stream state bits.
*
* "goodbit" means "everything looks ok" and has conflict with
* other bits (e.g. setting eof as true will make "good" false).
* "eofbit" means We are on End Of File and there is nothing left in buffer.
* "buffer_failbit" means tried to use buffer when buffer point was nullptr.
* "string_failbit" means there is failed operation at std::string related calls.
* "posixio_failbit" means a POSIX I/O function failed and errno was set.
*
* rdstate() function returns stream's current status based on this.
* setstate() function only accepts these values as its "bits".
*/
enum diostates
{
goodbit = 0x01,
eofbit = 0x02,
buffer_failbit = 0x04,
string_failbit = 0x08,
posixio_failbit = 0x10
};Also |
|
@mobin-2008 can you explain the streambuf changes to me please.
Why is that necessary? What is the benefit compared to the original design? |
|
streambuf is just a cpbuffer with fixed size. streambuf_base is a class which maintains a streambuf pointer and setbuf(), getbuf() functions to interact with that. It's designed to avoid duplication in ostream/istream. When maintaining streambuf in class itself is expensive and has some problems around the std::move, streambuf_allocator shows itself, streambuf_allocator is designed to create and maintain buffers outside of the class itself. |
Do we need streams to be movable? But alright, let's agree that it's probably better to keep the buffer outside the class because it's better to avoid large allocations on the stack. (And as a side benefit it's now feasible to move iostream objects, whether that's actually needed or not.) However, what is the point of streambuf_mgr? |
streambuf_allocator is an instance of streambuf_mgr. |
|
You are not actually explaining. I can see that |
|
What I mean is: why can't an iostream just allocate its own buffer? You can do dynamic allocation with |
|
About the moving object looks like I misunderstood about what you said:
I was thinking you meant there is should be move constructor anyway but looks like I was wrong. Hmm, This is why streambuf_mgr exist: If we don't have it, What can we do to keep buffers outside of classes? |
I guess by "buffers outside of classes" you mean buffers outside of the iostream objects. That is achieved by having a pointer or reference to the buffer instead of a buffer instance itself. For example: vs You've already got the latter (buffer separate the iostream objects) in your latest design. However, to allocate the streambuf the code currently does: My question is, what is the point of What is the advantage compared to: ? |
|
Right, I rewrited iostream buffers to use
|
It is better just to ask, if there is doubt. And you should try to understand the reasons rather than just follow the rule without question. In general in DInit we avoid doing dynamic allocation (eg It's probably better to use
These are equivalent. Please fix up things like this before asking me to look at it:
|
|
(Fundamentally I think the design is probably ok now. I haven't looked too closely because it is hard to interpret the design from the code and I don't want to have to review the code many times. I will review it properly when it is actually ready, so let me know). |
|
I can't understand what the hell just c++ standard did, |
|
Is it correct way to catch buf = std::unique_ptr<streambuf>(new(std::nothrow) streambuf);
if (buf == NULL) buf = nullptr; // Memory can't be allocated for new buffer, Failed |
|
Don't bother making
|
|
I refuse to use I think |
The
Do you mean because throwing exceptions can require heap allocation? We already use
That seems odd to me, what is being shared? Edit: Oh I see:
I don't understand this. How could this possibly be useful? I think you should avoid changing the design at this stage. We have gone over it and got something that seems sensible. Don't change it now!
I am ok with anything that makes sense and that uses the same principles used in the rest of the codebase (and which is idiomatic C++ unless there's a good reason to avoid that). Meaning: owning pointers should use |
|
I am reading the PR #263, and I wonder why libc isn't used for this purpose, since we already use |
Use of |
|
According to this reddit thread iostream will get replaced with some work on the way. Not sure, if any parts would be useful and how good the quality of the implementation is, because there is no CI and test coverage etc. |
Unfortunately, C++ iostream facilities suck. They are heavyweight due to supporting different character encodings. They make it impossible to get useful error messages in a standardised way (the language spec allows for it, but the implementations apparently don't bother and map all error conditions to the same useless message).
std::ios::failureextendsstd::system_errorbut doesn't useerrnoas the error code.We should replace them (including use of
cout/cerr) with a wrapper for POSIX file I/O.The text was updated successfully, but these errors were encountered: