On Tue, Sep 01, 2020 at 05:54:45PM +0200, Piotr Caban wrote:
Hi Arek,
The patch is quite long so I might have missed something.
Hey,
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.
+/* ??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?
+/* ??0ifstream@@QAE@PBDHH@Z */ +/* ??0ifstream@@QEAA@PEBDHH@Z */ +DEFINE_THISCALL_WRAPPER(ifstream_open_ctor, 20) +istream* __thiscall ifstream_open_ctor(istream *this, const char *name, ios_open_mode mode, int protection, BOOL virt_init) +{
- ios *base;
- filebuf *fb = MSVCRT_operator_new(sizeof(filebuf));
- TRACE("(%p %s %d %d %d)\n", this, name, mode, protection, virt_init);
- if (fb)
- {
filebuf_ctor(fb);istream_sb_ctor(this, &fb->base, virt_init);filebuf_open(fb, name, mode, protection);We should probably add OPENMODE_in as in e.g. msvcp90/ios.c:basic_ifstream_char_open. The same applies to all functions with mode argument.
This makes a lot of sense for an input stream. Same for ifstream_open. I've also added a few tests.
+/* ?attach@ifstream@@QAEXH@Z */ +/* ?attach@ifstream@@QEAAXH@Z */ +DEFINE_THISCALL_WRAPPER(ifstream_attach, 8) +void __thiscall ifstream_attach(istream *this, filedesc fd) +{
- TRACE("(%p %d)\n", this, fd);
- filebuf_attach(ifstream_rdbuf(this), fd);
+}
You should probably set failbit if filebuf_attach fails. The same applies to other functions on failure.
+/* ?close@ifstream@@QAEXXZ */ +/* ?close@ifstream@@QEAAXXZ */ +DEFINE_THISCALL_WRAPPER(ifstream_close, 4) +void __thiscall ifstream_close(istream *this) +{
- TRACE("(%p)\n", this);
- filebuf_close(ifstream_rdbuf(this));
+}
You should probably set failbit if filebuf_close fails. Also you should probably clear errors on successful close.
failbit handling added, with accompanying tests
- pifs = call_func5(p_ifstream_open_ctor, &ifs, filename, OPENMODE_in, filebuf_openprot, TRUE);
- pfb = (filebuf*) ifs.base_ios.sb;
- ok(ifs.base_ios.delbuf == 1, "expected 1 got %d\n", ifs.base_ios.delbuf);
- ok(pifs == &ifs, "wrong return, expected %p got %p\n", &ifs, pifs);
- ok(pfb->base.allocated == 1, "wrong allocate value, expected 1 got %d\n", pfb->base.allocated);
- ok(pfb->base.unbuffered == 0, "wrong unbuffered value, expected 0 got %d\n", pfb->base.unbuffered);
- ok(pfb->base.base != NULL, "wrong buffer, expected not %p got %p\n", NULL, pfb->base.base);
- ok(pfb->base.ebuf != NULL, "wrong ebuf, expected not %p got %p\n", NULL, pfb->base.ebuf);
- ok(pfb->fd != -1, "wrong fd, expected not -1 got %d\n", pfb->fd);
- fd = pfb->fd;
- ok(pfb->close == 1, "wrong value, expected 1 got %d\n", pfb->close);
- call_func1(p_ifstream_vbase_dtor, &ifs);
- ok(_close(fd) == -1, "expected ifstream to close opened file\n");
Isn't the fd closed by ifstream_vbase_dtor -> filebuf_dtor -> filebuf_close?
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?
- /* setbuf - seems to be a nop and always return NULL */
- pifs = call_func5(p_ifstream_buffer_ctor, &ifs, 64, NULL, 0, TRUE);
Passing arbitrary value as fd doesn't look right. How about changing 64 to -1? It will also show some problems with setbuf implementation.
-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.
Thanks for pointing out that setbuf behaves differently when we have no fd opened and no buffer.
Having done some experimentation it looks like filebuf has slightly different semantics than streambuf when it comes to setting unbuffered value in setbuf(). Wine's implemention doesn't follow that.
This was throwing me off while trying to make this work.
I'll leave setbuf() out for now, and spin another series fixing filebuf implementation first.
- psb = call_func3(p_ifstream_setbuf, &ifs, buffer, ARRAY_SIZE(buffer));
- ok(psb == NULL, "wrong return, expected NULL got %p\n", pfb);
Typo: pfb -> psb.
There are also some compilation warnings when compiling tests (i386).
fix'd :-)