[PATCH v2] msvcirt: Implement ifstream.

[Arkadiusz Hiler]

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 at HPADH@Z */
> > +/* ??0ifstream@@QEAA at 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 at PBDHH@Z */
> > +/* ??0ifstream@@QEAA at 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 at ifstream@@QAEXH at Z */
> > +/* ?attach at ifstream@@QEAAXH at 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 at ifstream@@QAEXXZ */
> > +/* ?close at 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 :-)

-- 
Cheers,
Arek