On 9/2/20 5:01 PM, Arkadiusz Hiler wrote:
Thanks for the review! I was wondering if I should split it a bit - ctros and dtors firts, and then each other method + correspnsidng tests in separate patches.
Not sure if this would really make a difference though.
It's easier to review smaller patches. I think it's OK to send it in one patch since you already have it written that way.
+/* ??0ifstream@@QAE@HPADH@Z */ +/* ??0ifstream@@QEAA@HPEADH@Z */ +DEFINE_THISCALL_WRAPPER(ifstream_buffer_ctor, 20) +istream* __thiscall ifstream_buffer_ctor(istream *this, filedesc fd, char *buffer, int length, BOOL virt_init) +{
- ios *base;
- filebuf *fb = MSVCRT_operator_new(sizeof(filebuf));
- TRACE("(%p %d %p %d %d)\n", this, fd, buffer, length, virt_init);
- if (fb)
- {
filebuf_fd_reserve_ctor(fb, fd, buffer, length);istream_sb_ctor(this, &fb->base, virt_init);- }
- else
istream_ctor(this, virt_init);The allocation failure handling looks quite strange. It's probably better to remove it so the application crashes instead of misbehaving.
I was wondering the same, but as Gijs have said other constructors seem to leave the object in that half initialized state (see ostrstream*_ctor, istrstream_buffer_ctor), so I went with it.
Should it instead set badbit or should it return NULL? Should we change all of the streams here?
I don't know how native handles out of memory error in this case. Newer versions are throwing bad_alloc exception but it was not available in msvcirt. I don't like the possibility of debugging the application only to find out that the ifstream object is broken due to out of memory condition. I would prefer something like: fb = MSVCRT_operator_new(sizeof(filebuf)); if (!fb) { FIXME("out of memory\n"); return NULL; } filebuf_fd_reserve_ctor(fb, fd, buffer, length); or even fb = MSVCRT_operator_new(sizeof(filebuf)); filebuf_fd_reserve_ctor(fb, fd, buffer, length);
It is. Here I am just making sure that everything works as expected on the higher level. Is using _close() to make sure that fd is closed wrong?
It's ok. I misread the code, sorry.
-1 have a special meaning, and I find ok() on the internal value with an arbitrary number a bit better when it come to testing. We do the same in filebuf tests and it works on Windows too.
It's generally nicer to pass valid file descriptor but it's not necessarily wrong. The risk is that the function will work differently due to fd validation. Anyway, since it's in tests, I don't really mind.