[cmake-developers] [PATCH v3 5/7] For consoles output on Windows use our own std::streambuf

[Dāvis Mosāns]

2016-07-07 1:36 GMT+03:00 Mike Gelfand <mikedld at mikedld.com>:
> On 07/07/2016 12:42 AM, clinton at elemtech.com wrote:
>> From what I remember, WriteConsoleW doesn't support redirection to a
>> file or pipe.  I don't see an alternative in the patch for the case
>> where stdout is not attached to the console.
> The SO thread I pointed at actually contained a check for whether
> corresponding stream is linked to terminal or not. Replacing stream
> buffers unconditionally is not right.
>

Sorry, I missed this... Will fix.

2016-07-07 2:21 GMT+03:00 Mike Gelfand <mikedld at mikedld.com>:
> Just a small nore: as I failed to mention this before, the approach
> taken in this patch will lead to messages to be printed to stdout and
> stderr in CMake's internal encoding (UTF-8?) when ConsoleBuf is not
> used, i.e. 1) on non-Windows and 2) on Windows with one or another
> stream redirected to file/pipe (considering corresponding check is in
> place). Seems to be the case anyway now, but maybe something to think about.
>

I know this, it always have been the case and with this patch I fix only
output to console because currently it would output internal encoding
which is unreadable for non-ASCII because console expects different codepage.

As for encoding used for files it doesn't matter because we use same one
for both writing and reading. It would matter if other applications would
want to read it. And because it's set as UTF-8 it should work fine.

> On 07/06/2016 10:12 PM, Dāvis Mosāns wrote:
>> --- a/Source/cmakemain.cxx
>> +++ b/Source/cmakemain.cxx
>> @@ -171,6 +189,16 @@ int main(int ac, char const* const* av)
>>  #ifdef CMAKE_BUILD_WITH_CMAKE
>>    cmDynamicLoader::FlushCache();
>>  #endif
>> +#if defined(_WIN32)
>> +  if (cbufio) {
>> +    delete cbufio;
>> +    std::cout.rdbuf(coutbuf);
>> +  }
>> +  if (cbuferr) {
>> +    delete cbuferr;
>> +    std::cerr.rdbuf(cerrbuf);
>> +  }
>> +#endif
>>    return ret;
>>  }
>>
> If exception was thrown in the beginning of main (as it seems you expect
> one there), `coutbuf` and/or `cerrbuf` could be nullptr, and passing
> nullptr here will fail.

coutbuf can be null only if

    cbufio = new cmsys::ConsoleBuf();

threw an exception in which case cbufio will be null and so it won't be set
here. And same for cerrbuf, it will be set if cbuferr is set in which case it
means there weren't any exception. While I think it's not needed, I can
add check for it and set rdbuf only when not null.

>
>> +        this->setg((char_type *)m_ibuffer.data(), (char_type *)m_ibuffer.data(), (char_type *)m_ibuffer.data() + m_ibuffer.size());
>> +        this->setp((char_type *)m_obuffer.data(), (char_type *)m_obuffer.data() + m_obuffer.size());
> This pattern over and over again. Needs to be put into separate member
> function, otherwise some day someone will mix "i" and "o" and wonder why
> doesn't it work.
>
> Also, I don't see that many calls to set(g|p) in that SO snipped, are
> they all really needed?
>

There aren't really that many calls, we set both once in constructor,
both once in sync and setg twice (differently) in underflow, because
two cases:

"The function may update gptr, egptr and eback pointers to define the
location of newly loaded data (if any). On failure, the function ensures
that either gptr() == nullptr or gptr() == egptr"

from http://en.cppreference.com/w/cpp/io/basic_streambuf/underflow