[PATCH v2] msvcirt: Implement ifstream.

[Piotr Caban]

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 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?
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.